[GitHub] [helix] rahulrane50 commented on a diff in pull request #2180: Reuse zkclient in BestPossibleExternalViewVerifier and fix resource leak

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

rahulrane50 commented on code in PR #2180:
URL: https://github.com/apache/helix/pull/2180#discussion_r933672737


##########
helix-core/src/main/java/org/apache/helix/tools/ClusterVerifiers/ZkHelixClusterVerifier.java:
##########
@@ -44,7 +44,7 @@
 import org.slf4j.LoggerFactory;
 
 public abstract class ZkHelixClusterVerifier
-    implements IZkChildListener, IZkDataListener, HelixClusterVerifier {
+    implements IZkChildListener, IZkDataListener, HelixClusterVerifier, AutoCloseable {

Review Comment:
   Let me clear my understanding here. I'm seeing that only place where _zkClient would be created now is any implementation of ZkHelixClusterVerifier which is BestPossibleExternalViewVerifier for this case. So now we appropriately passed usesExternalZkClient everywhere and now only place where _zkClient should be closed is either BestPossibleExternalViewVerifier or ZkHelixClusterVerifier. Now if we make ZkHelixClusterVerifier then won't it be little risky change since there are other implementations as well and i'm not sure what all things are shared in those instances. Rather than this making autoClosable here why can't we just override close in BestPossibleExternalViewVerifier and close zkClient if usesExternalZkClient is set to false. Let me know your thoughts



##########
helix-core/src/main/java/org/apache/helix/manager/zk/ZkBucketDataAccessor.java:
##########
@@ -135,6 +133,7 @@ private static RealmAwareZkClient createRealmAwareZkClient(String zkAddr) {
       zkClient = DedicatedZkClientFactory.getInstance()
           .buildZkClient(new HelixZkClient.ZkConnectionConfig(zkAddr));
     }
+    zkClient.setZkSerializer(new ByteArraySerializer());
     return zkClient;

Review Comment:
   Why did we changse serializer here? Previously it used to be ZkSerializer instance right?



##########
helix-core/src/main/java/org/apache/helix/tools/ClusterVerifiers/BestPossibleExternalViewVerifier.java:
##########
@@ -456,15 +450,17 @@ public String toString() {
        + (_resources != null ? Arrays.toString(_resources.toArray()) : "") + "])";
   }
 
+  // TODO: to clean up, finalize is deprecated in Java 9
   @Override
   public void finalize() {
+    super.finalize();
     close();
   }
 
-  private class DryrunWagedRebalancer extends org.apache.helix.controller.rebalancer.waged.ReadOnlyWagedRebalancer {
-    public DryrunWagedRebalancer(String metadataStoreAddress, String clusterName,
+  private static class DryrunWagedRebalancer extends ReadOnlyWagedRebalancer implements AutoCloseable {

Review Comment:
   Good call on Autoclosable! Just for my understanding why did we make static ?



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