xiangfu0 commented on code in PR #19926: URL: https://github.com/apache/pinot/pull/19926#discussion_r4367675061
########## pinot-common/src/main/java/org/apache/pinot/common/config/provider/BaseTableCache.java: ########## @@ -0,0 +1,345 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.pinot.common.config.provider; + +import java.util.ArrayList; +import java.util.HashMap; +import java.util.HashSet; +import java.util.List; +import java.util.Map; +import java.util.Set; +import java.util.concurrent.ConcurrentHashMap; +import java.util.stream.Collectors; +import javax.annotation.Nullable; +import org.apache.pinot.common.request.Expression; +import org.apache.pinot.spi.config.provider.LogicalTableConfigChangeListener; +import org.apache.pinot.spi.config.provider.SchemaChangeListener; +import org.apache.pinot.spi.config.provider.TableConfigChangeListener; +import org.apache.pinot.spi.config.table.TableConfig; +import org.apache.pinot.spi.data.LogicalTableConfig; +import org.apache.pinot.spi.data.Schema; +import org.apache.pinot.spi.utils.builder.TableNameBuilder; + + +/// Shared table, schema and logical-table cache for coordination adapters. +/// Query reads use concurrent maps; each metadata family is updated independently, not as a cluster-wide snapshot. +/// Adapters must hold the corresponding change lock through hydration, mutation and listener notification, and must Review Comment: Nice that the thread-safety contract is spelled out here. One addition worth considering for future backend authors: reads go through plain ConcurrentHashMaps without taking the change lock, so a reader can briefly observe a table config whose name-map entry is still mid-update (same as the old ZkTableCache, so not a regression). A sentence noting that reads give per-key, not snapshot, consistency would stop a future adapter from assuming otherwise. ########## pinot-common/src/main/java/org/apache/pinot/common/config/provider/ClusterConfigCache.java: ########## @@ -0,0 +1,65 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.pinot.common.config.provider; + +import java.util.ArrayList; +import java.util.List; +import java.util.Map; +import java.util.Set; +import org.apache.pinot.spi.config.provider.PinotClusterConfigChangeListener; +import org.apache.pinot.spi.config.provider.PinotClusterConfigProvider; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + + +/// Backend-neutral cache of cluster config snapshots. All access and listener callbacks are serialized on this +/// instance, so registration delivers the current snapshot before any later change. Backends supply complete snapshots +/// in order; this cache does not establish backend revisions or watch consistency. +public class ClusterConfigCache implements PinotClusterConfigProvider { + private static final Logger LOGGER = LoggerFactory.getLogger(ClusterConfigCache.class); + + private final List<PinotClusterConfigChangeListener> _listeners = new ArrayList<>(); + private Map<String, String> _clusterConfigs = Map.of(); + + /// Replaces the current snapshot and notifies registered listeners, including when no keys changed. Null-valued + /// entries are treated as absent. The supplied map is copied before publication. + public synchronized void updateClusterConfigs(Map<String, String> clusterConfigs) { + Map<String, String> snapshot = copyWithoutNullValues(clusterConfigs); + Set<String> changedConfigs = getChangedProperties(_clusterConfigs, snapshot); + LOGGER.info("Cluster configs changed: {}", changedConfigs); Review Comment: Minor: this logs every change at INFO (including the full changed-key set) and notifies listeners even when nothing actually changed. That faithfully preserves the old DefaultClusterConfigChangeHandler behavior, so no objection — just flagging that on a busy cluster this fires on every cluster-config write, so DEBUG might be worth considering if it ever gets noisy. -- 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]
