Jackie-Jiang commented on code in PR #19686:
URL: https://github.com/apache/pinot/pull/19686#discussion_r4186767196
##########
pinot-common/src/main/java/org/apache/pinot/common/config/DefaultClusterConfigChangeHandler.java:
##########
@@ -19,50 +19,112 @@
package org.apache.pinot.common.config;
import java.util.ArrayList;
+import java.util.Collections;
+import java.util.Comparator;
import java.util.List;
import java.util.Map;
import java.util.Set;
+import java.util.TreeMap;
import org.apache.helix.NotificationContext;
import org.apache.helix.api.listeners.BatchMode;
import org.apache.helix.api.listeners.ClusterConfigChangeListener;
import org.apache.helix.model.ClusterConfig;
import org.apache.pinot.spi.config.provider.PinotClusterConfigChangeListener;
import org.apache.pinot.spi.config.provider.PinotClusterConfigProvider;
+import org.apache.pinot.spi.env.PinotConfiguration;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
-/// Bridges Helix cluster config callbacks to
[PinotClusterConfigChangeListener]s. All access is serialized on this
-/// instance, so a listener sees the snapshot handed to it at registration and
every later change in order.
+/// Bridges Helix cluster config callbacks to
[PinotClusterConfigChangeListener]s. Instance configs captured at
+/// construction override cluster configs in the effective view exposed to
listeners and [#getClusterConfigs()]. All
+/// access is serialized on this instance, so a listener sees the snapshot
handed to it at registration and every later
+/// change in order.
@BatchMode(enabled = false)
public class DefaultClusterConfigChangeHandler implements
ClusterConfigChangeListener, PinotClusterConfigProvider {
private static final Logger LOGGER =
LoggerFactory.getLogger(DefaultClusterConfigChangeHandler.class);
+ private static final Comparator<String> CONFIG_KEY_COMPARATOR =
+
Comparator.comparing(DefaultClusterConfigChangeHandler::normalizeConfigKey);
private final List<PinotClusterConfigChangeListener> _listeners = new
ArrayList<>();
- private Map<String, String> _clusterConfigs = Map.of();
+ private final Map<String, String> _instanceConfigs;
+ private Map<String, String> _effectiveConfigs;
+
+ public DefaultClusterConfigChangeHandler() {
+ this(Map.of());
+ }
+
+ /// Snapshots the instance config at construction time. Callers must
construct the handler before folding cluster
+ /// configs into the instance config.
+ /// @param instanceConfig instance config to snapshot
+ public DefaultClusterConfigChangeHandler(PinotConfiguration instanceConfig) {
+ this(snapshot(instanceConfig));
+ }
+
+ DefaultClusterConfigChangeHandler(Map<String, String> instanceConfigs) {
+ _instanceConfigs = immutableConfigMap(instanceConfigs);
+ _effectiveConfigs = _instanceConfigs;
+ }
@Override
public synchronized void onClusterConfigChange(ClusterConfig clusterConfig,
NotificationContext context) {
Map<String, String> clusterConfigs =
copyWithoutNullValues(clusterConfig.getRecord().getSimpleFields());
- Set<String> changedConfigs = getChangedProperties(_clusterConfigs,
clusterConfigs);
+ Map<String, String> effectiveConfigs = getEffectiveConfigs(clusterConfigs);
+ Set<String> changedConfigs = getChangedProperties(_effectiveConfigs,
effectiveConfigs);
Review Comment:
[P2] Preserve normalized key matching in subsequent change notifications
Registration now passes the comparator-aware `_effectiveConfigs.keySet()`,
but `getChangedProperties()` returns an ordinary case-sensitive `HashSet` when
both views are nonempty. This makes a cluster key such as
`pinot.broker.query.option.validationmode=reject` work when
`QueryOptionConfigListener` registers after the first Helix callback, then
silently stop responding to changes: the listener checks the canonical
`validationMode` spelling, which the later HashSet does not recognize. Ordinary
instance properties keep the effective view nonempty even when the cluster key
is deleted, so removal also leaves the broker in REJECT instead of restoring
NONE.
A focused reproduction using this revision's handler and listener sources
produced:
| Key spelling | Startup | Changed to `warn` | Removed |
| --- | --- | --- | --- |
| `validationMode` | REJECT | WARN | NONE |
| `validationmode` | REJECT | REJECT | REJECT |
Please preserve the same normalized matching semantics in the changed-key
set, including removals, and add regression coverage for registration followed
by modification and deletion of an accepted alias.
--
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]
