[GitHub] [helix] qqu0127 commented on a diff in pull request #2291: Add Meta client factory and ZkMetaClient constructor

[GitBox <gi...@apache.org>]

qqu0127 commented on code in PR #2291:
URL: https://github.com/apache/helix/pull/2291#discussion_r1029923744


##########
meta-client/src/main/java/org/apache/helix/metaclient/constants/MetaClientConstants.java:
##########
@@ -0,0 +1,16 @@
+package org.apache.helix.metaclient.constants;
+
+public interface MetaClientConstants {

Review Comment:
   Also why do we make this class an interface?



##########
meta-client/src/main/java/org/apache/helix/metaclient/constants/MetaClientConstants.java:
##########
@@ -0,0 +1,16 @@
+package org.apache.helix.metaclient.constants;
+
+public interface MetaClientConstants {

Review Comment:
   Add apache license, same for a few other classes



##########
meta-client/src/main/java/org/apache/helix/metaclient/factories/MetaClientConfig.java:
##########
@@ -56,56 +69,81 @@ public StoreType getStoreType() {
   //  private RetryProtocol _retryProtocol;
 
 
-  private MetaClientConfig(String connectionAddress, long connectionTimeout, boolean enableAuth,
-      StoreType storeType) {
+  protected MetaClientConfig(String connectionAddress, long connectionInitTimeout,
+      long sessionTimeout, boolean enableAuth, StoreType storeType) {
     _connectionAddress = connectionAddress;
-    _connectionTimeout = connectionTimeout;
+    _connectionInitTimeout = connectionInitTimeout;
+    _sessionTimeout = sessionTimeout;
     _enableAuth = enableAuth;
     _storeType = storeType;
   }
 
-  public static class Builder {
-    private String _connectionAddress;
-
-    private long _connectionTimeout;
-    private boolean _enableAuth;
-    //private RetryProtocol _retryProtocol;
-    private StoreType _storeType;
+  public static class MetaClientConfigBuilder<B extends MetaClientConfigBuilder<B>> {

Review Comment:
   Could you please clarify why this generics is necessary? Thanks.



##########
meta-client/src/main/java/org/apache/helix/metaclient/impl/zk/ZkMetaClient.java:
##########
@@ -31,15 +31,21 @@
 import org.apache.helix.metaclient.api.DirectChildSubscribeResult;
 import org.apache.helix.metaclient.api.MetaClientInterface;
 import org.apache.helix.metaclient.api.OpResult;
-import org.apache.helix.zookeeper.api.client.RealmAwareZkClient;
+import org.apache.helix.metaclient.impl.zk.factory.ZkMetaClientConfig;
+import org.apache.helix.zookeeper.impl.client.ZkClient;
+import org.apache.helix.zookeeper.zkclient.ZkConnection;
 
 
 public class ZkMetaClient implements MetaClientInterface {
 
-  private RealmAwareZkClient _zkClient;
-
-  public ZkMetaClient() {
+  private ZkClient _zkClient;

Review Comment:
   can this be final?



##########
meta-client/src/main/java/org/apache/helix/metaclient/impl/zk/factory/ZkMetaClientFactory.java:
##########
@@ -0,0 +1,18 @@
+package org.apache.helix.metaclient.impl.zk.factory;
+
+import org.apache.helix.metaclient.api.MetaClientInterface;
+import org.apache.helix.metaclient.factories.MetaClientConfig;
+import org.apache.helix.metaclient.factories.MetaClientFactory;
+import org.apache.helix.metaclient.impl.zk.ZkMetaClient;
+
+
+public class ZkMetaClientFactory extends MetaClientFactory {
+  @Override
+  public MetaClientInterface getMetaClient(MetaClientConfig config) {
+    if (config.getStoreType() == MetaClientConfig.StoreType.ZOOKEEPER
+        && config instanceof ZkMetaClientConfig) {
+      return new ZkMetaClient((ZkMetaClientConfig) config);
+    }
+    return null;

Review Comment:
   I'm thinking if we should just throw `IllegalArgumentException` instead of return null. What's your thought on this?



##########
meta-client/src/main/java/org/apache/helix/metaclient/constants/MetaClientConstants.java:
##########
@@ -0,0 +1,16 @@
+package org.apache.helix.metaclient.constants;
+
+public interface MetaClientConstants {
+
+  // Stop retrying when we reach timeout
+  int DEFAULT_OPERATION_RETRY_TIMEOUT = Integer.MAX_VALUE;
+
+  // maxMsToWaitUntilConnected
+  int DEFAULT_CONNECTION_INIT_TIMEOUT = 60 * 1000;
+
+  // When a client becomes partitioned from the metadata service for more than session timeout,
+  // new session will be established.
+  int DEFAULT_SESSION_TIMEOUT = 30 * 1000;

Review Comment:
   Are these all in millisecond? 
   nit: maybe add comments or make it clear on the variable names?



##########
meta-client/src/main/java/org/apache/helix/metaclient/impl/zk/factory/ZkMetaClientConfig.java:
##########
@@ -0,0 +1,126 @@
+package org.apache.helix.metaclient.impl.zk.factory;
+
+import org.apache.helix.metaclient.factories.MetaClientConfig;
+import org.apache.helix.zookeeper.zkclient.serialize.BasicZkSerializer;
+import org.apache.helix.zookeeper.zkclient.serialize.PathBasedZkSerializer;
+import org.apache.helix.zookeeper.zkclient.serialize.SerializableSerializer;
+import org.apache.helix.zookeeper.zkclient.serialize.ZkSerializer;
+
+
+public class ZkMetaClientConfig extends MetaClientConfig {
+
+  protected PathBasedZkSerializer _zkSerializer;
+
+  // Monitoring related fields. MBean names are crated using following variables in format of
+  // MonitorPrefix_monitorType_monitorKey_monitorInstanceName, where _monitorInstanceName is optional
+  // TODO: right now all zkClient mBean object has prefix `HelixZkClient` had coded. We should change
+  // it to a configurable name.
+  protected String _monitorType;
+  protected String _monitorKey;
+  protected String _monitorInstanceName = null;
+  protected boolean _monitorRootPathOnly = true;

Review Comment:
   Usually with builder pattern, we can make member variables final. Thanks.



##########
meta-client/src/main/java/org/apache/helix/metaclient/constants/MetaClientConstants.java:
##########
@@ -0,0 +1,16 @@
+package org.apache.helix.metaclient.constants;
+
+public interface MetaClientConstants {
+
+  // Stop retrying when we reach timeout
+  int DEFAULT_OPERATION_RETRY_TIMEOUT = Integer.MAX_VALUE;

Review Comment:
   I think this value is inherited. But could we use a smaller timeout value for this? Open to discussion on this.



-- 
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: reviews-unsubscribe@helix.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org