xiangfu0 commented on code in PR #18286:
URL: https://github.com/apache/pinot/pull/18286#discussion_r3137567231
##########
pinot-core/src/main/java/org/apache/pinot/core/transport/server/routing/stats/ServerRoutingStatsManager.java:
##########
@@ -104,12 +115,42 @@ public void init() {
_enableStatsMetricExport =
_config.getProperty(AdaptiveServerSelector.CONFIG_OF_ENABLE_STATS_METRIC_EXPORT,
AdaptiveServerSelector.DEFAULT_ENABLE_STATS_METRIC_EXPORT);
- if (_enableStatsMetricExport) {
- long intervalMs =
_config.getProperty(AdaptiveServerSelector.CONFIG_OF_STATS_METRIC_EXPORT_INTERVAL_MS,
- AdaptiveServerSelector.DEFAULT_STATS_METRIC_EXPORT_INTERVAL_MS);
- _periodicTaskExecutor.scheduleAtFixedRate(this::exportStatsAsMetrics,
intervalMs, intervalMs,
- TimeUnit.MILLISECONDS);
- LOGGER.info("Adaptive server routing stats metric export enabled with
interval {}ms.", intervalMs);
+ _statsMetricExportIntervalMs =
_config.getProperty(AdaptiveServerSelector.CONFIG_OF_STATS_METRIC_EXPORT_INTERVAL_MS,
+ AdaptiveServerSelector.DEFAULT_STATS_METRIC_EXPORT_INTERVAL_MS);
+ _metricExportFuture =
_periodicTaskExecutor.scheduleAtFixedRate(this::exportStatsAsMetrics,
+ _statsMetricExportIntervalMs, _statsMetricExportIntervalMs,
TimeUnit.MILLISECONDS);
+ LOGGER.info("Adaptive server routing stats metric export scheduled with
interval {}ms (enabled={}).",
+ _statsMetricExportIntervalMs, _enableStatsMetricExport);
+ }
+
+ @Override
+ public void onChange(Set<String> changedConfigs, Map<String, String>
clusterConfigs) {
+ if
(changedConfigs.contains(AdaptiveServerSelector.CONFIG_OF_ENABLE_STATS_METRIC_EXPORT))
{
+ String value =
clusterConfigs.get(AdaptiveServerSelector.CONFIG_OF_ENABLE_STATS_METRIC_EXPORT);
+ if (value != null) {
+ _enableStatsMetricExport = Boolean.parseBoolean(value);
+ } else {
+ // Key was removed from cluster config — fall back to the static
broker config value.
+ _enableStatsMetricExport =
_config.getProperty(AdaptiveServerSelector.CONFIG_OF_ENABLE_STATS_METRIC_EXPORT,
+ AdaptiveServerSelector.DEFAULT_ENABLE_STATS_METRIC_EXPORT);
+ }
+ LOGGER.info("Updated enableStatsMetricExport to {} from cluster
config.", _enableStatsMetricExport);
+ if (!_enableStatsMetricExport) {
+ removeAllServerStatsGauges();
+ }
+ }
+ if
(changedConfigs.contains(AdaptiveServerSelector.CONFIG_OF_STATS_METRIC_EXPORT_INTERVAL_MS))
{
+ String value =
clusterConfigs.get(AdaptiveServerSelector.CONFIG_OF_STATS_METRIC_EXPORT_INTERVAL_MS);
+ long newIntervalMs = value != null ? Long.parseLong(value)
Review Comment:
This path trusts the cluster-config value blindly.
`DefaultClusterConfigChangeHandler.process()` invokes listeners inline, so a
bad value here (`abc`, `0`, `-1`) will throw out of the Helix callback via
`Long.parseLong` or `scheduleAtFixedRate`, and the update will never reach
later listeners. Since this PR makes the interval dynamically mutable, it needs
validation/fallback before mutating the existing schedule.
--
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]
