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 (after we drop the
resource, and before we check with an Assert statement) 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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
