HADOOP-17995. Stale record should be remove when DataNodePeerMetrics#dumpSendPacketDownstreamAvgInfoAsJson #3630
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…dumpSendPacketDownstreamAvgInfoAsJson
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
ferhui
reviewed
Nov 10, 2021
| @@ -167,7 +167,7 @@ synchronized void replaceScheduledTask(int windows, long interval, | |||
| } | |||
|
|
|||
| @Override | |||
| public void snapshot(MetricsRecordBuilder builder, boolean all) { | |||
| public synchronized void snapshot(MetricsRecordBuilder builder, boolean all) { | |||
There was a problem hiding this comment.
Do we need synchronized here? Its super class is not.
There was a problem hiding this comment.
@ferhui @tomscut Thanks your comment.
| SpotBugs | module:hadoop-common-project/hadoop-common |
|---|---|
| Inconsistent synchronization of org.apache.hadoop.metrics2.lib.MutableRollingAverages.recordValidityMs; locked 66% of time Unsynchronized access at MutableRollingAverages.java:66% of time Unsynchronized access at MutableRollingAverages.java:[line 182] |
So consider add synchronized.
Reference code MutableRatesWithAggregation#snapshot, the method to add the synchronized
public class MutableRatesWithAggregation extends MutableMetric {
@OverRide
public synchronized void snapshot(MetricsRecordBuilder rb, boolean all) {
Iterator<WeakReference<ConcurrentMap<String, ThreadSafeSampleStat>>> iter =
weakReferenceQueue.iterator();
...
}
}
There was a problem hiding this comment.
I'm not sure whether the synchronized is needed in here, the rest of part look good to me.
|
💔 -1 overall
This message was automatically generated. |
ferhui
approved these changes
Nov 15, 2021
tomscut
approved these changes
Nov 15, 2021
|
@haiyang1987 Thanks for contribution. @AlphaGouGe @tomscut Thanks for review! |
|
This change has introduced a findbugs warning on trunk, which was in the build above: I wonder if the snapshot method needs synchronised to avoid this? We cannot make |
|
or spotbugs has its rule changed. |
12 tasks
|
I don't think the rules changed. The final CI report on this PR has the new problem present, so I'm pretty sure this one introduced it. |
|
Revert this PR and investigate the findbugs warning. |
|
In the tests setRecordValidityMs method is set synchronized
So consider the snapshot method use recordValidityMs needs synchronised to avoid spotbugs warning " Unsynchronized access" ？ |
|
@haiyang1987 Thanks for your explanation. I think you are right, you can raise a New PR. |
HarshitGupta11
pushed a commit
to HarshitGupta11/hadoop
that referenced
this pull request
Nov 28, 2022
…dumpSendPacketDownstreamAvgInfoAsJson (apache#3630)
HarshitGupta11
pushed a commit
to HarshitGupta11/hadoop
that referenced
this pull request
Nov 28, 2022
…Metrics#dumpSendPacketDownstreamAvgInfoAsJson (apache#3630)" (apache#3697)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description of PR
As HADOOP-16947 problem with description.
Stale SumAndCount also should be remove when DataNodePeerMetrics#dumpSendPacketDownstreamAvgInfoAsJson.
Ensure the DataNode JMX get SendPacketDownstreamAvgInfo Metrics is accurate.
Details: HADOOP-17995