Follow-up on review comments for the bounded queue #2008
Conversation
jpkrohling
requested review from
black-adder,
objectiser,
pavolloffay,
tiffon,
vprithvi and
yurishkuro
as code owners
jpkrohling
changed the title
|
This is now ready for review. The only thing missing based on the review comments is to either use a mutex for touching the queue or wrapping it in I might be missing something, but I took another look at the code and couldn't find any serious consequences of a concurrent read while the new queue is being set. At most, we'd be getting an updated value for the capacity, which in the worst case will cause a span to be dropped (the other case is that the queues will have one more span than the desired capacity). If you think it's still worth having a lock, I can certainly add one for the |
Signed-off-by: Juraci Paixão Kröhling <juraci@kroehling.de>
Codecov Report
@@ Coverage Diff @@
## master #2008 +/- ##
==========================================
+ Coverage 96.93% 96.95% +0.02%
==========================================
Files 205 205
Lines 10114 10119 +5
==========================================
+ Hits 9804 9811 +7
+ Misses 271 268 -3
- Partials 39 40 +1
Continue to review full report at Codecov.
|
pkg/queue/bounded_queue.go
Outdated
| @@ -165,10 +170,13 @@ func (q *BoundedQueue) Resize(capacity int) bool { | |||
| swapped := atomic.CompareAndSwapPointer((*unsafe.Pointer)(unsafe.Pointer(&q.items)), unsafe.Pointer(q.items), unsafe.Pointer(&queue)) | |||
| if swapped { | |||
| // start a new set of consumers, based on the information given previously | |||
| q.StartConsumers(q.workers, q.consumer) | |||
| q.StartConsumers(int(q.workers), q.consumer) | |||
There was a problem hiding this comment.
Did you spot it using a different linter?
There was a problem hiding this comment.
We should plug your brain in the CI, as our linters didn't catch it.
Signed-off-by: Juraci Paixão Kröhling <juraci@kroehling.de>
Which problem is this PR solving?
Short description of the changes