narendly commented on a change in pull request #1056:
URL: https://github.com/apache/helix/pull/1056#discussion_r435511881



##########
File path: 
helix-core/src/main/java/org/apache/helix/monitoring/mbeans/JobMonitor.java
##########
@@ -182,6 +198,59 @@ public void updateControllerInducedDelayGauge(long delay) {
     _controllerInducedDelayGauge.updateValue(delay);
   }
 
+  /**
+   * Sync the current SubmissionToProcessDelay mean to ZooKeeper
+   * @param baseDataAccessor
+   */
+  public void syncSubmissionToProcessDelayToZk(BaseDataAccessor<ZNRecord> 
baseDataAccessor) {

Review comment:
       As discussed offline, this implementation could be improved.
   - This is too specific and not very easy to maintain or scalable - suppose 
you want to add more metrics that you need to persist, then you'd have to 
implement methods for each additional metric.
   - Can you look into inversion of control? Perhaps you could create a 
component with the appropriate set of interface methods that allow persisting 
of metric numbers to a metadata store in a more generic fashion. For example, 
you could have something like,
   
   > <I> MetricStorage
   > <Class> ZkMetricStorage (takes a Zk connection to initialize)
   
   Also, this MetricStorage implementation could take in a dynamic list of 
metric names to be persisted, etc. You could get creative with this. No need to 
feel like you have to cover all use cases, but designing a component in a way 
that is extendable and generic enough will save you and help with 
maintainability.




----------------------------------------------------------------
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]



---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Reply via email to