Add ability to update rate limiter #17
Conversation
Signed-off-by: Won Jun Jang <[email protected]>
Codecov Report
@@ Coverage Diff @@
## master #17 +/- ##
============================================
+ Coverage 84.77% 84.96% +0.19%
- Complexity 608 618 +10
============================================
Files 94 94
Lines 2410 2428 +18
Branches 271 271
============================================
+ Hits 2043 2063 +20
+ Misses 276 275 -1
+ Partials 91 90 -1
Continue to review full report at Codecov.
|
| return SamplingStatus.of(this.rateLimiter.checkCredit(1.0), tags); | ||
| } | ||
|
|
||
| @Override | ||
| public boolean equals(Object other) { | ||
| public synchronized boolean equals(Object other) { |
There was a problem hiding this comment.
Why does this need to be synchronized?
There was a problem hiding this comment.
If this function and the update function are called at the same time, there could(?) be a race condition on the variable maxTracesPerSecond. Might be overkill
| } | ||
|
|
||
| private double getMaxBalance(double maxTracesPerSecond) { | ||
| return maxTracesPerSecond < 1.0 ? 1.0 : maxTracesPerSecond; |
There was a problem hiding this comment.
Why 1.0? Could you refactor the magic number into a constant?
I'm not a fan of hardcoding this number here because we'll have to update the client when we need to change this number.
| private final Clock clock; | ||
| private double balance; | ||
| private double maxBalance; | ||
| private long lastTick; | ||
|
|
||
| public RateLimiter(double creditsPerSecond, double maxBalance) { | ||
| this(creditsPerSecond, maxBalance, new SystemClock()); | ||
| this(creditsPerSecond, maxBalance, new SystemClock(), maxBalance * new Random().nextDouble()); |
There was a problem hiding this comment.
Could we cache the Random()?
Signed-off-by: Won Jun Jang <[email protected]>
Signed-off-by: Won Jun Jang [email protected]
Which problem is this PR solving?
cf. jaegertracing/jaeger-client-go#320