MINOR: make Sensor#add idempotent#4853
MINOR: make Sensor#add idempotent#4853guozhangwang merged 3 commits intoapache:trunkfrom vvcephei:idempotent-sensor-add
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? |
bbejeck
left a comment
bbejeck
left a comment
There was a problem hiding this comment.
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. |
guozhangwang
left a comment
guozhangwang
left a comment
There was a problem hiding this comment.
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.
Should we log here indicating an add function called for the same metric name?
There was a problem hiding this comment.
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]>
4 participants
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)