-
Notifications
You must be signed in to change notification settings - Fork 232
Make GuaranteedThroughputSampler public #457
Conversation
Codecov Report
@@ Coverage Diff @@
## master #457 +/- ##
===========================================
+ Coverage 88.14% 88.25% +0.1%
+ Complexity 490 489 -1
===========================================
Files 62 62
Lines 1848 1847 -1
Branches 241 241
===========================================
+ Hits 1629 1630 +1
+ Misses 142 139 -3
- Partials 77 78 +1
Continue to review full report at Codecov.
|
@quaff, do you have a reason to require this to be public? Also, looks like your commit is missing the DCO sign-off. |
2dfbba9
to
4901f47
Compare
@jpkrohling I have updated this commit, I prefer this Sampler for my application, any concern to make it package access level? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really don't have any concerns, but we are about to discuss whether those samplers should be part of the public API or not. If you could share your use case, it would certainly help us decide to make this class part of it.
It would be really nice if you could edit this PR description to add your use case.
I'm reviewing this PR as "approved" and will merge as soon as you can share the description above. I'm sure nobody would have any concerns if you can provide such description :)
@@ -31,15 +30,15 @@ | |||
*/ | |||
@ToString | |||
@EqualsAndHashCode | |||
@AllArgsConstructor (access = AccessLevel.PACKAGE) // Visible for testing | |||
@AllArgsConstructor |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just noticed this change here: this exposes a new constructor, to initialize the private fields (probabilisticSampler
, lowerBoundSampler
and tags
).
I don't think we want that. In fact, I think this annotation can be removed altogether.
Dismissing the review, because of the all-args constructor
47d9c79
to
2182c26
Compare
@jpkrohling commit updated, since ProbabilisticSampler and RateLimitingSampler are public, why not GuaranteedThroughputSampler? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There might be a reason why this is not public. I'll ask about it during our meeting today, but I think nobody will object to merging this.
You are more than welcome to join our meeting today!
https://github.com/jaegertracing/jaeger#project-status-bi-weekly-meeting
@quaff Looks like a recent change caused merge conflicts with this PR. Would you please update it? Once it's rebased to the latest master, I'll merge it. |
Developers can use GuaranteedThroughputSampler as their sampler like ProbabilisticSampler or RateLimitingSampler. Signed-off-by: Yanming Zhou <zhouyanming@gmail.com>
@jpkrohling updated. |
Thanks, @quaff! |
No description provided.