kaisun2000 commented on a change in pull request #365: Fix RoutingTableProvider statePropagationLatency metric reporting bug URL: https://github.com/apache/helix/pull/365#discussion_r308446959
########## File path: helix-core/src/main/java/org/apache/helix/spectator/RoutingTableProvider.java ########## @@ -558,23 +558,28 @@ protected void refresh(Map<String, Map<String, Map<String, CurrentState>>> curre private void resetRoutingTableAndNotify(long startTime, RoutingTable newRoutingTable) { _routingTableRef.set(newRoutingTable); - logger.info("Refresh the RoutingTable for cluster {}, takes {} ms.", - (_helixManager != null ? _helixManager.getClusterName() : null), + String clusterName = _helixManager != null ? _helixManager.getClusterName() : null; + logger.info("Refresh the RoutingTable for cluster {}, takes {} ms.", clusterName, (System.currentTimeMillis() - startTime)); - notifyRoutingTableChange(); + + notifyRoutingTableChange(_helixManager != null ? _helixManager.getClusterName() : null); Review comment: notifyRoutingTableChange() is actually calling the user side registered callback function. The time spent here should be separately accounted for as this is out of our control. This can be potentially moved to some other place, for example in a separate thread specialized for this. That is the reason why I prefer to keep the interface separate, thus log separate. In fact, with testing going on with another team, we may need to put this logic into another thread later. ---------------------------------------------------------------- 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services