-
Notifications
You must be signed in to change notification settings - Fork 9.1k
HADOOP-18502. MutableStat should return 0 when there is no change #5058
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
Conversation
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
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.
@ted12138 Thanks for your contribution. This PR make sense.
Leave some minor comment. Please check it.
@ashutoshcipher Can help review this PR?
| } | ||
|
|
||
| /** | ||
| * MutableStat should output 0 instead of the previous state when there is no change |
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 end with .
| MetricsRecordBuilder builderWithoutChange = mockMetricsRecordBuilder(); | ||
| MetricsRegistry registry = new MetricsRegistry("test"); | ||
| MutableStat stat = registry.newStat("Test", "Test", "Ops", "Val", false); | ||
|
|
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 remove this empty line.
| assertGauge("TestAvgVal", 1.5, builderWithChange); | ||
|
|
||
| registry.snapshot(builderWithoutChange, true); | ||
| assertGauge("TestAvgVal", 0.0, builderWithoutChange); |
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.
Here too. Can add one UT to verify the INumOps?
| stat.add(1000, 2000); | ||
| registry.snapshot(builderWithChange, true); | ||
|
|
||
| assertCounter("TestNumOps", 2000L, builderWithChange); |
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 add one UT to verify the INumOps?
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.
Please add the requested UTs, rest it makes sense.
|
🎊 +1 overall
This message was automatically generated. |
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.
LGTM. Thanks @ted12138
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 @ted12138 for adding UT. LGTM +1
|
Merged. @ted12138 Thanks for your contribution. @ashutoshcipher Thanks for your review. |
…ache#5058) (Cherry-picked from 7002e21) ACLOVERRIDE
HADOOP-18502. MutableStat should return 0 when there is no change (apache#5058)
Description of PR
When we try to switch active NN to standby, we find that the getContentSummary average time is always a very high value even if there is no more query.
For us, the metrics return 0 is more reasonable.
How was this patch tested?
Active NN will hold some operations average response time, but when we switch ANN to SNN, the avg metrics should be 0.
For code changes:
LICENSE,LICENSE-binary,NOTICE-binaryfiles?