tomscut commented on code in PR #4186:
URL: https://github.com/apache/hadoop/pull/4186#discussion_r855696796
##########
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/blockmanagement/DatanodeManager.java:
##########
@@ -1856,15 +1875,16 @@ public DatanodeCommand[]
handleHeartbeat(DatanodeRegistration nodeReg,
nodeinfo.setBalancerBandwidth(0);
}
- if (slowPeerTracker != null) {
- final Map<String, Double> slowPeersMap = slowPeers.getSlowPeers();
- if (!slowPeersMap.isEmpty()) {
- if (LOG.isDebugEnabled()) {
- LOG.debug("DataNode " + nodeReg + " reported slow peers: " +
- slowPeersMap);
- }
- for (String slowNodeId : slowPeersMap.keySet()) {
- slowPeerTracker.addReport(slowNodeId, nodeReg.getIpcAddr(false));
+ synchronized (slowPeerTrackerLock) {
Review Comment:
Thanks @virajjasani for updating. Adding lock `slowPeerTrackerLock` looks
good. But I'm not sure if it affects heartbeat here.
I have one small suggestion:
1. Make dataNodePeerStatsEnabled volatile.
2. When dataNodePeerStatsEnabledVal = false, do not set slowPeerTracker to
null.
3. Change the condition from `if (slowPeerTracker != null)` to `if
(slowPeerTracker != null) && dataNodePeerStatsEnabled` where `slowPeerTracker`
is used.
Disadvantages:
if `dataNodePeerStatsEnabledVal = false`, `slowPeerTracker` will be
retained. But I guess the operation to `disable slowPeerTracker` is not common.
Advantages:
Does not cause lock contention and does not cause NPE.
What do you think of this?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
