suvodeep-pyne commented on code in PR #16823:
URL: https://github.com/apache/pinot/pull/16823#discussion_r2353216452
##########
pinot-broker/src/main/java/org/apache/pinot/broker/broker/helix/BaseBrokerStarter.java:
##########
@@ -129,6 +131,12 @@ public abstract class BaseBrokerStarter implements
ServiceStartable {
private volatile boolean _isStarting = false;
private volatile boolean _isShuttingDown = false;
+ // Class dedicated towards handling cluster config change
+ protected final DefaultClusterConfigChangeHandler
_defaultClusterConfigChangeHandler =
+ new DefaultClusterConfigChangeHandler();
+
+ // Deprecated in favor of using a dedicated
_defaultClusterConfigChangeHandler to manage config related changes
+ @Deprecated
Review Comment:
Thanks a lot for your diligence on this, I really appreciate the careful
review. 🙏 You’re absolutely right that the `ClusterChangeHandler` serves
broader use cases and brings additional capabilities like periodic syncs that
are valuable. I don’t want to take away from that at all.
I spent some time when deciding on whether to reuse the existing infra.
Where I’m coming from is more specific to the
`BaseBrokerStarter._clusterConfigChangeHandlers` list. My thinking is:
* **Centralized handling** – The `ClusterConfigChangeListener` approach
consolidates how cluster config changes are surfaced across controller, server,
and now broker, giving us a unified way to process them.
* **Dedicated interface** – It provides a clear, dedicated contract around
changed components and configs, which makes it easier for future contributors
to know upfront *what exactly has changed*.
* **Container for evolution** – Because it’s scoped specifically to cluster
config changes (rather than Helix changes in general), it can evolve
independently as we add new requirements.
* **Avoiding duplication** – Having both `_clusterConfigChangeHandlers` and
`_defaultClusterConfigChangeHandler` maintain parallel lists essentially
duplicates functionality. Consolidating into one prevents future contributors
from having to guess which path is the “right” one.
That’s why I suggested deprecating the list inside `BaseBrokerStarter`—not
the `ClusterChangeHandler` itself. In fact, I see value in keeping
`ClusterChangeHandler` for more general use cases around listening to cluster
"changes".
I completely hear your concern that we shouldn’t close off the
`ClusterChangeHandler` option prematurely. My view is that deprecating the list
in `BaseBrokerStarter` helps provide a clear instruction for new code while
still allowing folks to use `ClusterChangeHandler` where its broader
functionality is actually needed.
Happy to discuss on a call if you feel integrating the Audit code with the
existing mechanism is the better short-term compromise. My concern is that
leaving both lists side by side will make it harder for the next person to know
the preferred path forward.
What do you think?
--
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]
