[pkg/queue] Add StartConsumersWithFactory function#2714
Merged
yurishkuro merged 5 commits intojaegertracing:masterfrom Jan 7, 2021
Merged
[pkg/queue] Add StartConsumersWithFactory function#2714yurishkuro merged 5 commits intojaegertracing:masterfrom
StartConsumersWithFactory function#2714yurishkuro merged 5 commits intojaegertracing:masterfrom
Conversation
This provides a way to keep state for each consumer of a bounded queue, which is useful in certain performance-critical setups. Fixes jaegertracing#2685. Signed-off-by: Pablo Baeyens <pablo.baeyens@datadoghq.com>
Codecov Report
@@ Coverage Diff @@
## master #2714 +/- ##
==========================================
+ Coverage 95.73% 95.74% +0.01%
==========================================
Files 216 216
Lines 9588 9593 +5
==========================================
+ Hits 9179 9185 +6
Misses 336 336
+ Partials 73 72 -1
Continue to review full report at Codecov.
|
yurishkuro
reviewed
Jan 5, 2021
The common part with the assertions was moved to a new `checkQueue` function that is used by `TestBoundedQueue` and `TestBoundedQueueWithFactory`. Signed-off-by: Pablo Baeyens <pablo.baeyens@datadoghq.com>
yurishkuro
reviewed
Jan 6, 2021
yurishkuro
reviewed
Jan 6, 2021
yurishkuro
reviewed
Jan 6, 2021
Signed-off-by: Pablo Baeyens <pablo.baeyens@datadoghq.com>
Refactor unit tests using a `helper` function that takes a function to start consumers. Signed-off-by: Pablo Baeyens <pablo.baeyens@datadoghq.com>
Contributor
Author
|
@yurishkuro Thanks for the comments, I think I addressed everything, hopefully this looks better :) |
yurishkuro
approved these changes
Jan 7, 2021
| // We want to test the overflow behavior, so we block the consumer | ||
| // by holding a startLock before submitting items to the queue. | ||
| func TestBoundedQueue(t *testing.T) { | ||
| func helper(t *testing.T, startConsumers func(q *BoundedQueue, consumerFn func(item interface{}))) { |
Member
There was a problem hiding this comment.
hehe, I didn't mean the name helper literally, was just too lazy to type what you had, but that's fine.
Contributor
Author
There was a problem hiding this comment.
hahaha, I couldn't come up with anything better so I left it like you said 😄
| assert.False(t, q.Produce("x"), "cannot push to closed queue") | ||
| } | ||
|
|
||
| func TestBoundedQueue(t *testing.T) { |
Member
There was a problem hiding this comment.
much cleaner, minimal changes
bhiravabhatla
pushed a commit
to bhiravabhatla/jaeger
that referenced
this pull request
Jan 25, 2021
) * [pkg/queue] Add `StartConsumersWithFactory` function This provides a way to keep state for each consumer of a bounded queue, which is useful in certain performance-critical setups. Fixes jaegertracing#2685. Signed-off-by: Pablo Baeyens <pablo.baeyens@datadoghq.com> * Refactor bounded queue tests to extract common parts The common part with the assertions was moved to a new `checkQueue` function that is used by `TestBoundedQueue` and `TestBoundedQueueWithFactory`. Signed-off-by: Pablo Baeyens <pablo.baeyens@datadoghq.com> * Use http.HandlerFunc pattern for getting a Consumer from a callback Signed-off-by: Pablo Baeyens <pablo.baeyens@datadoghq.com> * Address review comment about unit tests Refactor unit tests using a `helper` function that takes a function to start consumers. Signed-off-by: Pablo Baeyens <pablo.baeyens@datadoghq.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Which problem is this PR solving?
Short description of the changes
StartConsumersWithFactoryfunction to allow for keeping state per-consumer.StartConsumerswith a dummy factory function.