qqu0127 commented on code in PR #2011:
URL: https://github.com/apache/helix/pull/2011#discussion_r841985016
##########
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:
Thanks for the comment!
> where would be the "correct" place for metric-cleanup logic to be?
With limited context and knowledge, I think both `ResourceValidationStage`
and `ExternalViewComputeStage` make sense. MetricsMonitors belong to, or at
least associated with resources. On the other hand, generally speaking, it's
also a "view".
it's also applicable to be in a standalone stage instead of mixing up with
other logic.
> async stage...
Yeah, this is a good point I wasn't aware of. So essentially, we'd want to
run it in async way, and make sure all related cluster events trigger such
computation.
> add a CurrentState change event to artificially trigger the stage
This is definitely an option for the fix. But could it introduce more
overhead and side effect? And what the difference from adding
`externalViewPipeline` to `IdealStateChange` registry? All we need is refresh
the resource monitors, but we will be running a few other pipelines.
--
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]
