virajjasani commented on code in PR #4186:
URL: https://github.com/apache/hadoop/pull/4186#discussion_r857307250
##########
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:
Will get back with adding new class `SlowPeerDisabledTracker` that extends
`SlowPeerTracker` such that all extended methods in SlowPeerDisabledTracker are
no-op, hence we never need to assign null to `slowPeerTracker` of
DatanodeManager in the first place and hence never need to deal with NPEs. This
will save us from using volatile or synchronization as well because both of
them would add to the performance of heartbeat handling.
Toggling b/ enabled and disabled mode of "slow peer tracking" would be much
simplified this way. Let me get back with the changes tomorrow.
Thanks @tomscut for the nice discussion!
--
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: common-issues-unsubscr...@hadoop.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: common-issues-h...@hadoop.apache.org
