-
Notifications
You must be signed in to change notification settings - Fork 13k
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
[FLINK-5261] Clean up meters in ScheduledDropwizardReporter #2944
Conversation
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.
Changes look good to me. +1 to merge after addressing my comments. Can you also open a PR for the backport of this to 1.1? Would like to have this in 1.1.4. (Just saw Chesnay's comment on the ML that we don't have meters in 1.1)
@@ -91,6 +91,10 @@ protected ScheduledDropwizardReporter() { | |||
return meters; | |||
} | |||
|
|||
Map<Gauge<?>, String> getGauges() { return gauges; } |
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.
Can you do a new line for the return and add a annotation like @VisibleForTesting
?
@@ -91,6 +91,10 @@ protected ScheduledDropwizardReporter() { | |||
return meters; | |||
} | |||
|
|||
Map<Gauge<?>, String> getGauges() { return gauges; } | |||
|
|||
Map<Histogram, String> getHistograms() { return histograms; } |
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.
Same here
@@ -124,6 +129,89 @@ public void testAddingMetrics() throws NoSuchFieldException, IllegalAccessExcept | |||
metricRegistry.shutdown(); | |||
} | |||
|
|||
@Test |
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.
Can you add a high level description about what this test does please? Otherwise this looks good.
@uce Thank you for reviewing this so quickly, I've addressed all your comments. |
Going to merge this with the next batch |
This PR fixes the metric-cleanup in the
ScheduledDropwizardReporter
. We were missing a separate branch for meters withinnotifyOfRemovedMetric
.I've also added a test case to make sure that all metric types are properly cleaned up, both on our and and from the Dropwizard registry.