kaisun2000 commented on a change in pull request #770: WIP: Add SharedZkClient and update SharedZkClientFactory URL: https://github.com/apache/helix/pull/770#discussion_r380961009
########## File path: zookeeper-api/src/main/java/org/apache/helix/zookeeper/impl/client/SharedZkClient.java ########## @@ -37,79 +50,541 @@ * HelixZkClient that uses shared ZkConnection. * A SharedZkClient won't manipulate the shared ZkConnection directly. */ -public class SharedZkClient extends ZkClient implements HelixZkClient { +public class SharedZkClient implements RealmAwareZkClient { private static Logger LOG = LoggerFactory.getLogger(SharedZkClient.class); - /* - * Since we cannot really disconnect the ZkConnection, we need a dummy ZkConnection placeholder. - * This is to ensure connection field is never null even the shared RealmAwareZkClient instance is closed so as to avoid NPE. - */ - private final static ZkConnection IDLE_CONNECTION = new ZkConnection("Dummy_ZkServers"); - private final OnCloseCallback _onCloseCallback; - private final ZkConnectionManager _connectionManager; - - public interface OnCloseCallback { - /** - * Triggered after the RealmAwareZkClient is closed. - */ - void onClose(); - } - - /** - * Construct a shared RealmAwareZkClient that uses a shared ZkConnection. - * - * @param connectionManager The manager of the shared ZkConnection. - * @param clientConfig ZkClientConfig details to create the shared RealmAwareZkClient. - * @param callback Clean up logic when the shared RealmAwareZkClient is closed. - */ - public SharedZkClient(ZkConnectionManager connectionManager, ZkClientConfig clientConfig, - OnCloseCallback callback) { - super(connectionManager.getConnection(), 0, clientConfig.getOperationRetryTimeout(), - clientConfig.getZkSerializer(), clientConfig.getMonitorType(), clientConfig.getMonitorKey(), - clientConfig.getMonitorInstanceName(), clientConfig.isMonitorRootPathOnly()); - _connectionManager = connectionManager; - // Register to the base dedicated RealmAwareZkClient - _connectionManager.registerWatcher(this); - _onCloseCallback = callback; + + private final HelixZkClient _innerSharedZkClient; + private final Map<String, String> _routingDataCache; // TODO: replace with RoutingDataCache + private final String _zkRealmShardingKey; + + public SharedZkClient(RealmAwareZkClient.RealmAwareZkConnectionConfig connectionConfig, + RealmAwareZkClient.RealmAwareZkClientConfig clientConfig, + Map<String, String> routingDataCache) { + + if (connectionConfig == null) { + throw new IllegalArgumentException("RealmAwareZkConnectionConfig cannot be null!"); + } + _zkRealmShardingKey = connectionConfig.getZkRealmShardingKey(); + + // TODO: Replace this Map with a real RoutingDataCache + if (routingDataCache == null) { + throw new IllegalArgumentException("RoutingDataCache cannot be null!"); + } + _routingDataCache = routingDataCache; + + // Get the ZkRealm address based on the ZK path sharding key + String zkRealmAddress = _routingDataCache.get(_zkRealmShardingKey); + + // Create an InnerSharedZkClient to actually serve ZK requests + // TODO: Rename HelixZkClient in the future or remove it entirely - this will be a backward-compatibility breaking change because HelixZkClient is being used by Helix users. + HelixZkClient.ZkConnectionConfig zkConnectionConfig = + new HelixZkClient.ZkConnectionConfig(zkRealmAddress) + .setSessionTimeout(connectionConfig.getSessionTimeout()); + _innerSharedZkClient = SharedZkClientFactory.getInstance() Review comment: This actually implies the ZKRealmAware SharedZKClient are mixed with the original SharedZKCLient (now called innerSharedZKClient). We should note this behavior. Put it another way, underlying the same ZkConnection manager, there can be innersharedZkClient which do not check path validity and new SharedZkClient which does check path validity. ---------------------------------------------------------------- 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services --------------------------------------------------------------------- To unsubscribe, e-mail: reviews-unsubscr...@helix.apache.org For additional commands, e-mail: reviews-h...@helix.apache.org