New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
KAFKA-10458; Updating controller quota does not work since Token Bucket #9272
Conversation
…ating the MetricConfig of the KafkaMetric is reflected in the Sensor.
} | ||
|
||
public Stat stat() { | ||
return this.stat; |
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.
nit: this
is not really needed, since it's not ambiguous here as in constructor,
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.
Thanks for finding a way to fix this without changing Sensor and/or Metrics API. Looks great!
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.
} | ||
|
||
public MetricConfig config() { | ||
return this.configSupplier.get(); |
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 guess if you are removing this.
above, you could remove it here as well for consistency.
} | ||
|
||
public MetricConfig config() { | ||
return this.configSupplier.get(); |
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 guess if you are removing this.
above, you could remove it here as well for consistency.
@apovzner @rajinisivaram Thanks for your feedback. I have addressed your comments. |
@dajac Thanks for the update, test failures not related, merging to trunk. |
This PR fixes two issues that have been introduced by #9114.
When I switched the metric from
Rate
toTokenBucket
in theControllerMutationQuotaManager
, I have mixed up the metrics. That broke the quota update path.When a quota is updated, the
ClientQuotaManager
updates theMetricConfig
of theKafkaMetric
. That update was not reflected into theSensor
so theSensor
was still using theMetricConfig
that it has been created with.Committer Checklist (excluded from commit message)