Use hit counter to track max QPS per minute for broker #4472
Conversation
| * gets called, we put the timestamp to the specified bucket. When the method getHitCount gets called, we sum all the number | ||
| * of hits within the last 100 time buckets. | ||
| * This hit counter is for counting the number of hits within a range of time. | ||
| * Right now the granularity we use is configured the users. Currently two users |
There was a problem hiding this comment.
"configured by the users"? I think we can omit this, since the ctor kind of indicates that the granularity is used by whoever calls the constructor. The use of the term "user" here could somehow indicate the user of pinot, or administrator of pinot -- neither of which is true in this case.
|
|
||
| /* | ||
| * Explanation of algorithm to keep track of | ||
| * max QPS per minute: |
There was a problem hiding this comment.
"max QPS within a one minute window"?
There was a problem hiding this comment.
yep, corrected it
| * and each bucket covers a fixed time window to | ||
| * cover an overall range of 60secs. | ||
| * | ||
| * Each bucket stores two values -- start time |
There was a problem hiding this comment.
it seems that we only need to store the start time for the first bucket, right? the others can be derived. Not sure if we implement it that way, just making a note here.
There was a problem hiding this comment.
I am not quite sure what that implies so we should discuss it. Let's take an example:
Start time of bucket b1 is 100 and bucket b2 is 101. Now if there are 6 buckets, then 106 and 107 will also map to b1 and b2 respectively.
When there are only 2 hits recorded for 100 and 101 start times the latter can probably be derived from former. But let's say 3rd hit comes after a window of 1 min and let say start time is 107 then b2's start time will be set to 107 and this is something that can't be derived from b1's start time of 100 (although b1's hit count has now become irrelelevant as it is not out of 60sec window)
| * every time a query comes into {@link org.apache.pinot.broker.requesthandler.BaseBrokerRequestHandler} | ||
| */ | ||
| public void hitAndUpdateLatestTime() { | ||
| hitAndUpdateLatestTime(System.currentTimeMillis()); |
There was a problem hiding this comment.
If it helps, you can introduce a protected method called nowMillis() which returns current time in millis. You can then override this method to return whatever time you like in tests. Yet to read through the rest of the review, but just suggesting here. Not sure if it will help us get better test coverage.
There was a problem hiding this comment.
For coverage, I am explicitly calling the next method with a custom timestamp to test various values at different intervals.
|
Addressed review comments |
| } | ||
|
|
||
| @VisibleForTesting | ||
| void hitAndUpdateLatestTime(final long timestamp) { |
There was a problem hiding this comment.
Use the same as the existing method to hit()
|
Closed this PR due to 6 months inactivity. Reopen if needed. |
Using the hit counter in a configured manner (600 buckets of 100ms each to cover a time range of 60sec) to keep track of max QPS per minute for broker.
Detailed explanation of algorithm is there is in HitCounter.java