narendly commented on a change in pull request #632: Asynchronously calculating the Baseline URL: https://github.com/apache/helix/pull/632#discussion_r351631243
########## File path: helix-core/src/main/java/org/apache/helix/monitoring/metrics/implementation/RebalanceLatencyGauge.java ########## @@ -30,6 +31,7 @@ private static final Logger LOG = LoggerFactory.getLogger(RebalanceLatencyGauge.class); private static final long VALUE_NOT_SET = -1; private long _lastEmittedMetricValue = VALUE_NOT_SET; + private final ThreadLocal<Long> _startTime; Review comment: Why does this need to be ThreadLocal? My guess is because you need to make sure the latency metric is strictly linearized (as in they cannot overlap). But this is a guess based on the context I have. I do not think the code itself is self-explanatory. - Craftsmanship/Documentation: There is no clear description for any of this. Do you agree that using ThreadLocal is a little odd for metrics, and that we need to make ourselves clear on why this is needed and what the implication is? - Code review guidelines: this seems like a change that could have been in a separate PR in order to make the review process go faster and smoother. The more modular you make the changes, the better review you'll get, and you'll produce a better solution to weight-based rebalancing. Please consider breaking this change up. This should precede any other pipeline-related changes. I will consider adding a point in our code review/PR guidelines: keep changes to a minimum and modular. ---------------------------------------------------------------- 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: [email protected] With regards, Apache Git Services --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
