[GitHub] [zookeeper] TisonKun commented on a change in pull request #1095: ZOOKEEPER-837: Eliminate cycle dependency between ClientCnxn and ZooKeeper

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

TisonKun commented on a change in pull request #1095: ZOOKEEPER-837: Eliminate cycle dependency between ClientCnxn and ZooKeeper
URL: https://github.com/apache/zookeeper/pull/1095#discussion_r368800174
 
 

 ##########
 File path: zookeeper-server/src/main/java/org/apache/zookeeper/ClientCnxn.java
 ##########
 @@ -352,35 +354,29 @@ public String toString() {
      * established until needed. The start() instance method must be called
      * subsequent to construction.
      *
-     * @param chrootPath - the chroot of this client. Should be removed from this Class in ZOOKEEPER-838
-     * @param hostProvider
-     *                the list of ZooKeeper servers to connect to
-     * @param sessionTimeout
-     *                the timeout for connections.
-     * @param zooKeeper
-     *                the zookeeper object that this connection is related to.
-     * @param watcher watcher for this connection
-     * @param clientCnxnSocket
-     *                the socket implementation used (e.g. NIO/Netty)
-     * @param canBeReadOnly
-     *                whether the connection is allowed to go to read-only
-     *                mode in case of partitioning
-     * @throws IOException
+     * @param chrootPath the chroot of this client. Should be removed from this Class in ZOOKEEPER-838
+     * @param hostProvider the list of ZooKeeper servers to connect to
+     * @param sessionTimeout the timeout for connections.
+     * @param clientConfig the client configuration.
+     * @param defaultWatcher default watcher for this connection
+     * @param clientCnxnSocket the socket implementation used (e.g. NIO/Netty)
+     * @param canBeReadOnly whether the connection is allowed to go to read-only mode in case of partitioning
      */
     public ClientCnxn(
         String chrootPath,
         HostProvider hostProvider,
         int sessionTimeout,
-        ZooKeeper zooKeeper,
-        ClientWatchManager watcher,
+        ZKClientConfig clientConfig,
 
 Review comment:
   @eolivelli After a close look I find that where Curator hacking into `CliengCnxn` is only `InjectSessionExpiration` that get access of `cnxn`/`eventThread`/`queueEvent`. So this patch should not break Curator.
   
   And I second @Randgalt that it is Curator responsibility to adjust its trick for accessing ZooKeeper internal concept since they don't promise compatibility. IMO Curator already has a `Compatibility` utils for different manner between zk34 and zk35. We should continue refining codebase so that projects such as Curator don't have to rely on internal concepts, though.

----------------------------------------------------------------
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