You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@helix.apache.org by GitBox <gi...@apache.org> on 2020/02/14 03:51:26 UTC

[GitHub] [helix] kaisun2000 opened a new pull request #763: SharedZKClient 

kaisun2000 opened a new pull request #763: SharedZKClient 
URL: https://github.com/apache/helix/pull/763
 
 
   ### Issues
   
   - [ x ] My PR addresses the following Helix issues and references them in the PR description:
   
   Resolve #762 
   
   
   ### Description
   
   - [ ] Here are some details about my PR, including screenshots of any UI changes:
   
   This diff added the implementation of building realm aware shared ZKClient implementation to ShareZkClientFactory.
   
   The diff intends to achieve this with minimal change and easy to reason correctness in terms of keeping the zk client session management, sync and async semantic and auto-reconnection upon session expiration intact.
   
   ### Tests
   
   - [ ] The following tests are written for this issue:
   
   To be done.
   
   - [x ] The following is the result of the "mvn test" command on the appropriate module:
   
   "mvn -pl zookeeper-api test" pass though it does not seem to invoke any real test.
   
   ### Commits
   
   - [ ] My commits all reference appropriate Apache Helix GitHub issues in their subject lines, and I have squashed multiple commits if they address the same issue. In addition, my commits follow the guidelines from "[How to write a good git commit message](http://chris.beams.io/posts/git-commit/)":
     1. Subject is separated from body by a blank line
     1. Subject is limited to 50 characters (not including Jira issue reference)
     1. Subject does not end with a period
     1. Subject uses the imperative mood ("add", not "adding")
     1. Body wraps at 72 characters
     1. Body explains "what" and "why", not "how"
   
   ### Documentation
   
   - [ ] In case of new functionality, my PR adds documentation in the following wiki page:
   
   (Link the GitHub wiki you added)
   
   ### Code Quality
   
   - [ ] My diff has been formatted using helix-style.xml

----------------------------------------------------------------
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:
users@infra.apache.org


With regards,
Apache Git Services

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

[GitHub] [helix] narendly commented on issue #763: SharedZKClient

Posted by GitBox <gi...@apache.org>.
narendly commented on issue #763: SharedZKClient 
URL: https://github.com/apache/helix/pull/763#issuecomment-589986996
 
 
   @kaisun2000 Could you please go ahead and close this PR if it's no longer valid? Thanks!

----------------------------------------------------------------
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:
users@infra.apache.org


With regards,
Apache Git Services

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

[GitHub] [helix] dasahcc commented on a change in pull request #763: SharedZKClient

Posted by GitBox <gi...@apache.org>.
dasahcc commented on a change in pull request #763: SharedZKClient 
URL: https://github.com/apache/helix/pull/763#discussion_r379661309
 
 

 ##########
 File path: zookeeper-api/src/main/java/org/apache/helix/zookeeper/impl/client/SharedZkClient.java
 ##########
 @@ -72,6 +72,25 @@ public SharedZkClient(ZkConnectionManager connectionManager, ZkClientConfig clie
     _onCloseCallback = callback;
   }
 
+  /**
+   * 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(String realmKey, ZkConnectionManager connectionManager, RealmAwareZkClientConfig clientConfig,
 
 Review comment:
   Can we have consistent pattern of constructor for DedicatedZkClient and SharedZKClient? This constructor is different from @narendly 's constructor.

----------------------------------------------------------------
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:
users@infra.apache.org


With regards,
Apache Git Services

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

[GitHub] [helix] narendly commented on a change in pull request #763: SharedZKClient

Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #763: SharedZKClient 
URL: https://github.com/apache/helix/pull/763#discussion_r379660504
 
 

 ##########
 File path: zookeeper-api/src/main/java/org/apache/helix/zookeeper/impl/client/SharedZkClient.java
 ##########
 @@ -72,6 +72,25 @@ public SharedZkClient(ZkConnectionManager connectionManager, ZkClientConfig clie
     _onCloseCallback = callback;
   }
 
+  /**
+   * 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(String realmKey, ZkConnectionManager connectionManager, RealmAwareZkClientConfig clientConfig,
 
 Review comment:
   As we discussed, SharedZkClient is supposed to implement RealmAwareZkClient interface.

----------------------------------------------------------------
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:
users@infra.apache.org


With regards,
Apache Git Services

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

[GitHub] [helix] dasahcc commented on a change in pull request #763: SharedZKClient

Posted by GitBox <gi...@apache.org>.
dasahcc commented on a change in pull request #763: SharedZKClient 
URL: https://github.com/apache/helix/pull/763#discussion_r379660611
 
 

 ##########
 File path: zookeeper-api/src/main/java/org/apache/helix/zookeeper/api/factory/RealmAwareZkClientFactory.java
 ##########
 @@ -40,6 +40,8 @@ RealmAwareZkClient buildZkClient(RealmAwareZkClient.RealmAwareZkConnectionConfig
    * @param connectionConfig
    * @return RealmAwareZkClient
    */
-  RealmAwareZkClient buildZkClient(
-      RealmAwareZkClient.RealmAwareZkConnectionConfig connectionConfig);
+  default RealmAwareZkClient buildZkClient(
+      RealmAwareZkClient.RealmAwareZkConnectionConfig connectionConfig) {
+    return buildZkClient(connectionConfig, new RealmAwareZkClient.RealmAwareZkClientConfig());
 
 Review comment:
   Is this default impl necessary?

----------------------------------------------------------------
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:
users@infra.apache.org


With regards,
Apache Git Services

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

[GitHub] [helix] narendly commented on a change in pull request #763: SharedZKClient

Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #763: SharedZKClient 
URL: https://github.com/apache/helix/pull/763#discussion_r379660897
 
 

 ##########
 File path: zookeeper-api/src/main/java/org/apache/helix/zookeeper/impl/client/ZkClient.java
 ##########
 @@ -90,7 +90,15 @@ public ZkClient(IZkConnection zkConnection, int connectionTimeout, long operatio
       PathBasedZkSerializer zkSerializer,
       String monitorType, String monitorKey, String monitorInstanceName,
       boolean monitorRootPathOnly) {
-    super(zkConnection, connectionTimeout, operationRetryTimeout, zkSerializer, monitorType,
+    super(null, zkConnection, connectionTimeout, operationRetryTimeout, zkSerializer, monitorType,
+        monitorKey, monitorInstanceName, monitorRootPathOnly);
+  }
+
+  public ZkClient(String realmKey, IZkConnection zkConnection, int connectionTimeout, long operationRetryTimeout,
 
 Review comment:
   I don't think making the raw ZkClient realm-aware is appropriate because it's supposed to be a "raw" ZkClient that preserves the behavior of what we had in I0Itec's ZkClient.

----------------------------------------------------------------
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:
users@infra.apache.org


With regards,
Apache Git Services

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

[GitHub] [helix] narendly commented on issue #763: SharedZKClient

Posted by GitBox <gi...@apache.org>.
narendly commented on issue #763: SharedZKClient 
URL: https://github.com/apache/helix/pull/763#issuecomment-586485892
 
 
   @kaisun2000 Please update the title of the PR. "SharedZkClient" does not conform to the convention.

----------------------------------------------------------------
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:
users@infra.apache.org


With regards,
Apache Git Services

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

[GitHub] [helix] kaisun2000 commented on issue #763: SharedZKClient

Posted by GitBox <gi...@apache.org>.
kaisun2000 commented on issue #763: SharedZKClient 
URL: https://github.com/apache/helix/pull/763#issuecomment-591089005
 
 
   This is an approach of an alternative design. We decided to use composition/delegation. Thus close this one. 

----------------------------------------------------------------
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:
users@infra.apache.org


With regards,
Apache Git Services

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

[GitHub] [helix] narendly commented on a change in pull request #763: SharedZKClient

Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #763: SharedZKClient 
URL: https://github.com/apache/helix/pull/763#discussion_r379661215
 
 

 ##########
 File path: zookeeper-api/src/main/java/org/apache/helix/zookeeper/zkclient/ZkClient.java
 ##########
 @@ -170,13 +172,21 @@ public void recordPathStat(Stat stat, OptionalLong notificationTime) {
       }
     }
   }
-
   protected ZkClient(IZkConnection zkConnection, int connectionTimeout, long operationRetryTimeout,
       PathBasedZkSerializer zkSerializer, String monitorType, String monitorKey,
       String monitorInstanceName, boolean monitorRootPathOnly) {
+    this(null, zkConnection, connectionTimeout, operationRetryTimeout,
+        zkSerializer, monitorType, monitorKey,
+        monitorInstanceName, monitorRootPathOnly);
+  }
+
+  protected ZkClient(String realmKey, IZkConnection zkConnection, int connectionTimeout, long operationRetryTimeout,
 
 Review comment:
   This is the native ZkClient. I don't think this is a good idea to touch the native ZkClient for the reasons stated above.

----------------------------------------------------------------
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:
users@infra.apache.org


With regards,
Apache Git Services

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

[GitHub] [helix] kaisun2000 closed pull request #763: SharedZKClient

Posted by GitBox <gi...@apache.org>.
kaisun2000 closed pull request #763: SharedZKClient 
URL: https://github.com/apache/helix/pull/763
 
 
   

----------------------------------------------------------------
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:
users@infra.apache.org


With regards,
Apache Git Services

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

[GitHub] [helix] narendly commented on issue #763: SharedZKClient

Posted by GitBox <gi...@apache.org>.
narendly commented on issue #763: SharedZKClient 
URL: https://github.com/apache/helix/pull/763#issuecomment-589379121
 
 
   I don't see any update on this PR? @kaisun2000 

----------------------------------------------------------------
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:
users@infra.apache.org


With regards,
Apache Git Services

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