-
Notifications
You must be signed in to change notification settings - Fork 13.9k
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
MINOR: make Sensor#add idempotent #4853
Conversation
I decided to pull this change into a separate PR. Who else should I ask for a review? |
@wicknicks Can you also take a look at this proposal? |
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.
Overall lgtm. I don't have that much experience with metrics, but what if someone wants to update or change a metric with the same name? How would that be accomplished?
failures are related; unused import in |
@bbejeck Thanks for the review. You can't update or change a metric, even today. You would have to remove the old metric and then add a new one. This should work the same before as after this PR. |
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.
One nit comment, otherwise LGTM.
this.stats.add(stat); | ||
return true; | ||
} else if (metrics.containsKey(metricName)) { | ||
return true; |
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.
Should we log here indicating an add
function called for the same metric name?
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.
This behavior is similar to the idempotent sensor creation at https://github.com/vvcephei/kafka/blob/1578db793d54482adaa6a90d4ca1fce45afb929a/clients/src/main/java/org/apache/kafka/common/metrics/Metrics.java#L399
Interestingly, the choice there was to log only when the sensor got created, not when it was a no-op.
I'm open to whatever.
the tests actually passed, but jenkins got rate-limited from Github. Retest this, please. |
This change makes adding a metric to a sensor idempotent. That is, if the metric is already added to the sensor, the method returns with success. The current behavior is that any attempt to register a second metric with the same name is an error. Testing strategy: There is a new unit test covering this behavior Reviewers: Guozhang Wang <[email protected]>, Bill Bejeck <[email protected]>
This change makes adding a metric to a sensor idempotent. That is, if the metric is already added to the sensor, the method returns with success. The current behavior is that any attempt to register a second metric with the same name is an error. Testing strategy: There is a new unit test covering this behavior Reviewers: Guozhang Wang <[email protected]>, Bill Bejeck <[email protected]>
This change makes adding a metric to a sensor idempotent. That is, if the metric is already added to the sensor, the method returns with success. The current behavior is that any attempt to register a second metric with the same name is an error. Testing strategy: There is a new unit test covering this behavior Reviewers: Guozhang Wang <[email protected]>, Bill Bejeck <[email protected]>
This change makes adding a metric to a sensor idempotent. That is, if the metric is already added to the sensor, the method returns with success. The current behavior is that any attempt to register a second metric with the same name is an error. Testing strategy: There is a new unit test covering this behavior Reviewers: Guozhang Wang <[email protected]>, Bill Bejeck <[email protected]>
This change makes adding a metric to a sensor idempotent.
That is, if the metric is already added to the sensor, the method
returns with success.
The current behavior is that any attempt to register a second metric
with the same name is an error.
Testing strategy: There is a new unit test covering this behavior
Committer Checklist (excluded from commit message)