narendly commented on a change in pull request #908: Make Helix REST realm-aware URL: https://github.com/apache/helix/pull/908#discussion_r398066168
########## File path: helix-rest/src/main/java/org/apache/helix/rest/server/ServerContext.java ########## @@ -20,42 +20,67 @@ * under the License. */ -import java.util.HashMap; +import java.io.IOException; +import java.util.List; import java.util.Map; +import java.util.concurrent.ConcurrentHashMap; import org.apache.helix.ConfigAccessor; import org.apache.helix.HelixAdmin; import org.apache.helix.HelixDataAccessor; +import org.apache.helix.HelixException; import org.apache.helix.InstanceType; -import org.apache.helix.rest.metadatastore.ZkMetadataStoreDirectory; -import org.apache.helix.zookeeper.datamodel.ZNRecord; +import org.apache.helix.SystemPropertyKeys; import org.apache.helix.manager.zk.ZKHelixAdmin; import org.apache.helix.manager.zk.ZKHelixDataAccessor; -import org.apache.helix.manager.zk.ZNRecordSerializer; import org.apache.helix.manager.zk.ZkBaseDataAccessor; -import org.apache.helix.zookeeper.impl.client.ZkClient; -import org.apache.helix.zookeeper.api.client.HelixZkClient; -import org.apache.helix.zookeeper.impl.factory.SharedZkClientFactory; +import org.apache.helix.msdcommon.exception.InvalidRoutingDataException; +import org.apache.helix.rest.metadatastore.ZkMetadataStoreDirectory; +import org.apache.helix.rest.metadatastore.accessor.ZkRoutingDataReader; import org.apache.helix.task.TaskDriver; import org.apache.helix.tools.ClusterSetup; +import org.apache.helix.zookeeper.api.client.HelixZkClient; +import org.apache.helix.zookeeper.api.client.RealmAwareZkClient; +import org.apache.helix.zookeeper.datamodel.ZNRecord; +import org.apache.helix.zookeeper.datamodel.serializer.ZNRecordSerializer; +import org.apache.helix.zookeeper.impl.client.FederatedZkClient; +import org.apache.helix.zookeeper.impl.client.ZkClient; +import org.apache.helix.zookeeper.impl.factory.DedicatedZkClientFactory; +import org.apache.helix.zookeeper.impl.factory.SharedZkClientFactory; +import org.apache.helix.zookeeper.util.HttpRoutingDataReader; +import org.apache.helix.zookeeper.zkclient.IZkChildListener; +import org.apache.helix.zookeeper.zkclient.IZkDataListener; +import org.apache.helix.zookeeper.zkclient.IZkStateListener; import org.apache.helix.zookeeper.zkclient.exception.ZkMarshallingError; import org.apache.helix.zookeeper.zkclient.serialize.ZkSerializer; +import org.apache.zookeeper.Watcher; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; -public class ServerContext { +public class ServerContext implements IZkDataListener, IZkChildListener, IZkStateListener { + private static final Logger LOG = LoggerFactory.getLogger(ServerContext.class); + private final String _zkAddr; - private HelixZkClient _zkClient; - private ZKHelixAdmin _zkHelixAdmin; - private ClusterSetup _clusterSetup; - private ConfigAccessor _configAccessor; + private volatile RealmAwareZkClient _zkClient; + + private volatile ZKHelixAdmin _zkHelixAdmin; + private volatile ClusterSetup _clusterSetup; + private volatile ConfigAccessor _configAccessor; // A lazily-initialized base data accessor that reads/writes byte array to ZK // TODO: Only read (deserialize) is supported at this time. This baseDataAccessor should support write (serialize) as needs arise private volatile ZkBaseDataAccessor<byte[]> _byteArrayZkBaseDataAccessor; // 1 Cluster name will correspond to 1 helix data accessor private final Map<String, HelixDataAccessor> _helixDataAccessorPool; // 1 Cluster name will correspond to 1 task driver private final Map<String, TaskDriver> _taskDriverPool; Review comment: I understand your concern and where you're coming from. But it's still difficult. TaskDriver and ConfigAccessor are different APIs by design. - TaskDriver/HelixManager/HelixDataAccessor -> only serve 1 single Helix cluster - ConfigAccessor/ClusterSetup/etc.. -> can serve many Helix clusters So we need to keep a pool regardless. To help you understand: ``` public TaskDriver(HelixManager manager) { } @Deprecated public TaskDriver(RealmAwareZkClient client, String clusterName) { } @Deprecated public TaskDriver(RealmAwareZkClient client, ZkBaseDataAccessor<ZNRecord> baseAccessor, String clusterName) {} @Deprecated public TaskDriver(HelixAdmin admin, HelixDataAccessor accessor, ConfigAccessor cfgAccessor, HelixPropertyStore<ZNRecord> propertyStore, String clusterName) { this(admin, accessor, propertyStore, clusterName); } ``` You see that every constructor of TaskDriver takes in a cluster name. Although TaskDriver does not support any ephemeral operations, by design it's meant to be a single cluster API. I think what you wanted to see was to pass in some FederatedZkClient into TaskDriver and use one TaskDriver for all clusters, but 1) that does not work, and 2) if we wanted to make that work, that's going to require a lot of code change in TaskDriver. ---------------------------------------------------------------- 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