siddharthteotia commented on a change in pull request #4472: Use hit counter to
track max QPS per minute for broker
URL: https://github.com/apache/incubator-pinot/pull/4472#discussion_r308470755
##########
File path:
pinot-broker/src/main/java/org/apache/pinot/broker/queryquota/HitCounter.java
##########
@@ -77,11 +90,154 @@ public int getHitCount() {
int getHitCount(long timestamp) {
long numTimeUnits = timestamp / _timeBucketWidthMs;
int count = 0;
- for (int i = 0; i < BUCKET_COUNT; i++) {
- if (numTimeUnits - _bucketStartTime.get(i) < BUCKET_COUNT) {
+ for (int i = 0; i < _bucketCount; i++) {
+ if (numTimeUnits - _bucketStartTime.get(i) < _bucketCount) {
count += _bucketHitCount.get(i);
}
}
return count;
}
+
+ /*
+ * Explanation of algorithm to keep track of
+ * max QPS per minute:
+ *
+ * Hit counter is configured with fixed number buckets
+ * and each bucket covers a fixed time window to
+ * cover an overall range of 60secs.
+ *
+ * Each bucket stores two values -- start time
+ * and hit counter.
+ *
+ * Example:
+ * T = timestamp
+ * BST = bucket start time
+ * B = bucket index
+ * WIDTH = bucket window width = 10000ms
+ * BUCKETS = 6
+ *
+ * Every time hitAndUpdateLatestTime() is called,
+ * we do the following:
+ *
+ * T = current timestamp
+ * BST = T / WIDTH
+ * B = BST % BUCKETS
+ *
+ * if a timestamp T is ending in a bucket B,
+ * T, T + 0, T + 1, ..... T + 9999ms will all
+ * end up in the same bucket B
+ *
+ * T + WIDTHms will end up in the next bucket
+ * and so on
+ *
+ * T + 60000, T + 60000 + 1 ... T + 60000 + 9999
+ * will also end up in the same bucket B as T but
+ * BST would be T + BUCKETS
+ *
+ * Now this is how bucket update rules work on every
+ * call to hitAndUpdateLatesTime()
+ *
+ * (1) Get T
+ * (2) Compute BST and B
+ * (3) CURR = current BST of B
+ * (4) update B's start time as BST
+ * (5) if BST != CURR, it implies we have gone
+ * over a minute and the current hit counter value of the
+ * bucket along with CURR can be overwritten --
+ * so we set B's hit counter to 1 and B's BST to BST.
+ * else, we increment B's hit counter by 1
+ *
+ * note that BST != CURR also implies that
+ * BST - CURR >= BUCKETS which further indicates we have gone
+ * over a minute.
+ *
+ * getMaxHitCount() -- used to update BrokerGauge with peak
+ * QPS per minute
+ *
+ * (1) go over all buckets
+ * (2) keep track of 2 max values seen so far in the loop
+ * (a) max_BST and (b) max_hits
+ * (3) consider each bucket's BST to see if max_BST
+ * should be updated
+ * (4) consider each bucket's hit counter value
+ * as follows to see if max_hits need to be updated
+ *
+ * if abs(bucket's BST - max_BST) <= BUCKETS, it
+ * tell us that start time of this bucket is within
+ * the window of 1min, so we should consider it's hit
+ * counter value to update max_hits
+ *
+ * else if bucket's BST > max_BST, it implies
+ * that bucket's BST is beyond current max_BST
+ * by more than a minute, so we should simply
+ * set max_hits as bucket's hit counter value
+ *
+ * See the unit test for peakQPS in HitCounterTest
+ */
+
+ /**
+ * Currently invoked by {@link
org.apache.pinot.broker.requesthandler.BrokerPeakQPSMetricsHandler}
+ * every time a query comes into {@link
org.apache.pinot.broker.requesthandler.BaseBrokerRequestHandler}
+ */
+ public void hitAndUpdateLatestTime() {
+ hitAndUpdateLatestTime(System.currentTimeMillis());
Review comment:
For coverage, I am explicitly calling the next method with a custom
timestamp to test various values at different intervals.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
