[GitHub] [helix] narendly commented on a diff in pull request #2011: WIP -- Attempt to fix TestDropResourceMetricsReset

[GitBox <gi...@apache.org>]

narendly commented on code in PR #2011:
URL: https://github.com/apache/helix/pull/2011#discussion_r841954676


##########
helix-core/src/main/java/org/apache/helix/controller/stages/ExternalViewComputeStage.java:
##########
@@ -96,7 +96,7 @@ public void execute(final ClusterEvent event) throws Exception {
 
     // Keep MBeans for existing resources and unregister MBeans for dropped resources
     if (clusterStatusMonitor != null) {
-      clusterStatusMonitor.retainResourceMonitor(monitoringResources);
+      //clusterStatusMonitor.retainResourceMonitor(monitoringResources);

Review Comment:
   First off, great work on debugging this and identifying the root cause!
   
   Have a few points to add:
   
   - First, with a guiding question: conceptually, where would be the "correct" place for metric-cleanup logic to be? 
   - We want the metric cleanup logic to be in an async stage in order to not block the critical controller pipeline. As such, adding the metric-cleanup logic in ResourceValidation may not be appropriate (this is not an async stage). If you believe that this logic shouldn't belong in ExternalViewComputeStage, then can we think about a different (or a new) async stage where this logic can be appropriately placed? With that said though, if you place it in another async stage, would we still face the same issue of this logic not triggering in tests?
   - If you think ExternalViewComputeStage is an appropriate place for this cleanup logic, can we simply add a CurrentState change event to artificially trigger the stage just for the sake of making the test case work? I think this is also a valid way because in real-life scenarios we can rely on the ExtViewComputeStage to eventually get called asynchronously. We can think of this as making the test environment model the real environment more closely.
   



-- 
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: reviews-unsubscribe@helix.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org