You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by GitBox <gi...@apache.org> on 2020/09/03 06:06:55 UTC
[GitHub] [kafka] apovzner opened a new pull request #9249: KAFKA-10458: Add update quota for TokenBucket registered with Sensor
apovzner opened a new pull request #9249:
URL: https://github.com/apache/kafka/pull/9249
For Rate() metric with quota config, we update quota by updating config of KafkaMetric. However, it is not enough for TokenBucket, because it uses quota config on record() to properly calculate the number of tokens. Sensor passes config stored in the corresponding StatAndConfig, which currently never changes. This means that after updating quota via KafkaMetric.config, which is our current and only method, Sensor would record the value using old quota but then measure the value to check for quota violation using the new quota value. This PR adds update method to Sensor that properly updates quota for TokenBucket.
### Committer Checklist (excluded from commit message)
- [ ] Verify design and implementation
- [ ] Verify test coverage and CI build status
- [ ] Verify documentation (including upgrade notes)
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [kafka] dajac commented on pull request #9249: KAFKA-10458: Add update quota for TokenBucket registered with Sensor
Posted by GitBox <gi...@apache.org>.
dajac commented on pull request #9249:
URL: https://github.com/apache/kafka/pull/9249#issuecomment-686334772
@apovzner Thanks for the PR. I will review it asap.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [kafka] gwenshap commented on a change in pull request #9249: KAFKA-10458: Add update quota for TokenBucket registered with Sensor
Posted by GitBox <gi...@apache.org>.
gwenshap commented on a change in pull request #9249:
URL: https://github.com/apache/kafka/pull/9249#discussion_r484602439
##########
File path: clients/src/main/java/org/apache/kafka/common/metrics/Sensor.java
##########
@@ -324,11 +332,35 @@ public synchronized boolean add(final MetricName metricName, final MeasurableSta
);
registry.registerMetric(metric);
metrics.put(metric.metricName(), metric);
- stats.add(new StatAndConfig(Objects.requireNonNull(stat), statConfig));
+ stats.add(new StatAndConfig(Objects.requireNonNull(stat), statConfig, metricName));
return true;
}
}
+ /**
+ * Update config of a measurable stat and metric registered with this sensor that corresponds to a given metric name
+ * @param metricName The name of the metric to update
+ * @param config New configuration for this metric
+ * @return true if config was updated, false if sensor expired or metric is not registered with the sensor
+ */
+ public synchronized boolean update(final MetricName metricName, final MetricConfig config) {
Review comment:
nit: The method signature for StatAndConfig has MetricConfig parameter first and MetricName second. This one has the reverse order.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [kafka] apovzner closed pull request #9249: KAFKA-10458: Add update quota for TokenBucket registered with Sensor
Posted by GitBox <gi...@apache.org>.
apovzner closed pull request #9249:
URL: https://github.com/apache/kafka/pull/9249
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [kafka] dajac commented on pull request #9249: KAFKA-10458: Add update quota for TokenBucket registered with Sensor
Posted by GitBox <gi...@apache.org>.
dajac commented on pull request #9249:
URL: https://github.com/apache/kafka/pull/9249#issuecomment-689564871
@apovzner I have looked at this further and I have thought about another approach. I have opened a PR for it: https://github.com/apache/kafka/pull/9272. I hope that you don't mind. Could you take a look so we can discuss both approaches and decide what is best?
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [kafka] apovzner commented on pull request #9249: KAFKA-10458: Add update quota for TokenBucket registered with Sensor
Posted by GitBox <gi...@apache.org>.
apovzner commented on pull request #9249:
URL: https://github.com/apache/kafka/pull/9249#issuecomment-689715022
@dajac I like your approach better. I will close this PR.
----------------------------------------------------------------
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:
users@infra.apache.org