You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@helix.apache.org by hu...@apache.org on 2021/01/06 19:36:53 UTC
[helix] branch master updated: Fix ZkClient leakage by correctly
setting usesExternalZkClient flag (#1562)
This is an automated email from the ASF dual-hosted git repository.
hulee pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/helix.git
The following commit(s) were added to refs/heads/master by this push:
new 6de54ab Fix ZkClient leakage by correctly setting usesExternalZkClient flag (#1562)
6de54ab is described below
commit 6de54ab5c35637accebe0dd819e946f79cabcceb
Author: Hunter Lee <na...@gmail.com>
AuthorDate: Wed Jan 6 11:36:43 2021 -0800
Fix ZkClient leakage by correctly setting usesExternalZkClient flag (#1562)
ZkBaseDataAccessor and Helix API that uses it using its Builder were having zkClient thread leak issues because the usesExternalZkClient flag was not being set properly. Also, the family of Verifiers were having a similar issue. This change solves this by making sure the private/protected constructors used by the Builders of these classes set the flag correctly to false so that in their respective close() functions, the underlying zkClient (created by the Builder) would get closed.
---
.../helix/manager/zk/ZkBaseDataAccessor.java | 13 ++++++------
.../BestPossibleExternalViewVerifier.java | 14 +++++++++----
.../ClusterVerifiers/ClusterLiveNodesVerifier.java | 8 ++++++--
.../StrictMatchExternalViewVerifier.java | 23 +++++++++++++++++-----
.../ClusterVerifiers/ZkHelixClusterVerifier.java | 11 ++++++-----
5 files changed, 47 insertions(+), 22 deletions(-)
diff --git a/helix-core/src/main/java/org/apache/helix/manager/zk/ZkBaseDataAccessor.java b/helix-core/src/main/java/org/apache/helix/manager/zk/ZkBaseDataAccessor.java
index 4e6a7b0..2721640 100644
--- a/helix-core/src/main/java/org/apache/helix/manager/zk/ZkBaseDataAccessor.java
+++ b/helix-core/src/main/java/org/apache/helix/manager/zk/ZkBaseDataAccessor.java
@@ -134,14 +134,13 @@ public class ZkBaseDataAccessor<T> implements BaseDataAccessor<T> {
*/
@Deprecated
public ZkBaseDataAccessor(RealmAwareZkClient zkClient) {
- if (zkClient == null) {
- throw new NullPointerException("zkclient is null");
- }
- _zkClient = zkClient;
- _usesExternalZkClient = true;
+ this(zkClient, true);
}
private ZkBaseDataAccessor(RealmAwareZkClient zkClient, boolean usesExternalZkClient) {
+ if (zkClient == null) {
+ throw new IllegalArgumentException("zkclient is null");
+ }
_zkClient = zkClient;
_usesExternalZkClient = usesExternalZkClient;
}
@@ -1317,9 +1316,11 @@ public class ZkBaseDataAccessor<T> implements BaseDataAccessor<T> {
*/
public ZkBaseDataAccessor<T> build() {
validate();
+ // Initialize ZkBaseDataAccessor with usesExternalZkClient = false so that
+ // ZkBaseDataAccessor::close() would close ZkClient as well to prevent thread leakage
return new ZkBaseDataAccessor<>(
createZkClient(_realmMode, _realmAwareZkConnectionConfig, _realmAwareZkClientConfig,
- _zkAddress));
+ _zkAddress), false);
}
}
diff --git a/helix-core/src/main/java/org/apache/helix/tools/ClusterVerifiers/BestPossibleExternalViewVerifier.java b/helix-core/src/main/java/org/apache/helix/tools/ClusterVerifiers/BestPossibleExternalViewVerifier.java
index c984d38..f916583 100644
--- a/helix-core/src/main/java/org/apache/helix/tools/ClusterVerifiers/BestPossibleExternalViewVerifier.java
+++ b/helix-core/src/main/java/org/apache/helix/tools/ClusterVerifiers/BestPossibleExternalViewVerifier.java
@@ -111,7 +111,9 @@ public class BestPossibleExternalViewVerifier extends ZkHelixClusterVerifier {
public BestPossibleExternalViewVerifier(RealmAwareZkClient zkClient, String clusterName,
Set<String> resources, Map<String, Map<String, String>> errStates,
Set<String> expectLiveInstances, int waitTillVerify) {
- super(zkClient, clusterName, waitTillVerify);
+ // usesExternalZkClient = true because ZkClient is given by the caller
+ // at close(), we will not close this ZkClient because it might be being used elsewhere
+ super(zkClient, clusterName, true, waitTillVerify);
_errStates = errStates;
_resources = resources;
_expectLiveInstances = expectLiveInstances;
@@ -120,8 +122,10 @@ public class BestPossibleExternalViewVerifier extends ZkHelixClusterVerifier {
private BestPossibleExternalViewVerifier(RealmAwareZkClient zkClient, String clusterName,
Map<String, Map<String, String>> errStates, Set<String> resources,
- Set<String> expectLiveInstances, int waitPeriodTillVerify) {
- super(zkClient, clusterName, waitPeriodTillVerify);
+ Set<String> expectLiveInstances, int waitPeriodTillVerify, boolean usesExternalZkClient) {
+ // Initialize BestPossibleExternalViewVerifier with usesExternalZkClient = false so that
+ // BestPossibleExternalViewVerifier::close() would close ZkClient to prevent thread leakage
+ super(zkClient, clusterName, usesExternalZkClient, waitPeriodTillVerify);
// Deep copy data from Builder
_errStates = new HashMap<>();
if (errStates != null) {
@@ -139,6 +143,7 @@ public class BestPossibleExternalViewVerifier extends ZkHelixClusterVerifier {
private Set<String> _resources;
private Set<String> _expectLiveInstances;
private RealmAwareZkClient _zkClient;
+ private boolean _usesExternalZkClient = false; // false by default
public Builder(String clusterName) {
_clusterName = clusterName;
@@ -164,7 +169,7 @@ public class BestPossibleExternalViewVerifier extends ZkHelixClusterVerifier {
return new BestPossibleExternalViewVerifier(
createZkClient(RealmAwareZkClient.RealmMode.SINGLE_REALM, _realmAwareZkConnectionConfig,
_realmAwareZkClientConfig, _zkAddress), _clusterName, _errStates, _resources,
- _expectLiveInstances, _waitPeriodTillVerify);
+ _expectLiveInstances, _waitPeriodTillVerify, _usesExternalZkClient);
}
public String getClusterName() {
@@ -204,6 +209,7 @@ public class BestPossibleExternalViewVerifier extends ZkHelixClusterVerifier {
public Builder setZkClient(RealmAwareZkClient zkClient) {
_zkClient = zkClient;
+ _usesExternalZkClient = true; // Set the flag since external ZkClient is used
return this;
}
}
diff --git a/helix-core/src/main/java/org/apache/helix/tools/ClusterVerifiers/ClusterLiveNodesVerifier.java b/helix-core/src/main/java/org/apache/helix/tools/ClusterVerifiers/ClusterLiveNodesVerifier.java
index 8e28f3e..789b509 100644
--- a/helix-core/src/main/java/org/apache/helix/tools/ClusterVerifiers/ClusterLiveNodesVerifier.java
+++ b/helix-core/src/main/java/org/apache/helix/tools/ClusterVerifiers/ClusterLiveNodesVerifier.java
@@ -34,13 +34,17 @@ public class ClusterLiveNodesVerifier extends ZkHelixClusterVerifier {
@Deprecated
public ClusterLiveNodesVerifier(RealmAwareZkClient zkclient, String clusterName,
List<String> expectLiveNodes) {
- super(zkclient, clusterName, 0);
+ // usesExternalZkClient = true because ZkClient is given by the caller
+ // at close(), we will not close this ZkClient because it might be being used elsewhere
+ super(zkclient, clusterName, true, 0);
_expectLiveNodes = new HashSet<>(expectLiveNodes);
}
private ClusterLiveNodesVerifier(RealmAwareZkClient zkClient, String clusterName,
Set<String> expectLiveNodes, int waitPeriodTillVerify) {
- super(zkClient, clusterName, waitPeriodTillVerify);
+ // Initialize ClusterLiveNodesVerifier with usesExternalZkClient = false so that
+ // ClusterLiveNodesVerifier::close() would close ZkClient to prevent thread leakage
+ super(zkClient, clusterName, false, waitPeriodTillVerify);
_expectLiveNodes = expectLiveNodes == null ? new HashSet<>() : new HashSet<>(expectLiveNodes);
}
diff --git a/helix-core/src/main/java/org/apache/helix/tools/ClusterVerifiers/StrictMatchExternalViewVerifier.java b/helix-core/src/main/java/org/apache/helix/tools/ClusterVerifiers/StrictMatchExternalViewVerifier.java
index bb991bd..ca47c16 100644
--- a/helix-core/src/main/java/org/apache/helix/tools/ClusterVerifiers/StrictMatchExternalViewVerifier.java
+++ b/helix-core/src/main/java/org/apache/helix/tools/ClusterVerifiers/StrictMatchExternalViewVerifier.java
@@ -66,7 +66,13 @@ public class StrictMatchExternalViewVerifier extends ZkHelixClusterVerifier {
@Deprecated
public StrictMatchExternalViewVerifier(RealmAwareZkClient zkClient, String clusterName,
Set<String> resources, Set<String> expectLiveInstances) {
- this(zkClient, clusterName, resources, expectLiveInstances, false, 0);
+ // usesExternalZkClient = true because ZkClient is given by the caller
+ // at close(), we will not close this ZkClient because it might be being used elsewhere
+ super(zkClient, clusterName, true, 0);
+ _resources = resources == null ? new HashSet<>() : new HashSet<>(resources);
+ _expectLiveInstances =
+ expectLiveInstances == null ? new HashSet<>() : new HashSet<>(expectLiveInstances);
+ _isDeactivatedNodeAware = false;
}
@Deprecated
@@ -79,8 +85,11 @@ public class StrictMatchExternalViewVerifier extends ZkHelixClusterVerifier {
}
private StrictMatchExternalViewVerifier(RealmAwareZkClient zkClient, String clusterName,
- Set<String> resources, Set<String> expectLiveInstances, boolean isDeactivatedNodeAware, int waitPeriodTillVerify) {
- super(zkClient, clusterName, waitPeriodTillVerify);
+ Set<String> resources, Set<String> expectLiveInstances, boolean isDeactivatedNodeAware,
+ int waitPeriodTillVerify, boolean usesExternalZkClient) {
+ // Initialize StrictMatchExternalViewVerifier with usesExternalZkClient = false so that
+ // StrictMatchExternalViewVerifier::close() would close ZkClient to prevent thread leakage
+ super(zkClient, clusterName, usesExternalZkClient, 0);
_resources = resources == null ? new HashSet<>() : new HashSet<>(resources);
_expectLiveInstances =
expectLiveInstances == null ? new HashSet<>() : new HashSet<>(expectLiveInstances);
@@ -94,6 +103,7 @@ public class StrictMatchExternalViewVerifier extends ZkHelixClusterVerifier {
private RealmAwareZkClient _zkClient;
// For backward compatibility, set the default isDeactivatedNodeAware to be false.
private boolean _isDeactivatedNodeAware = false;
+ private boolean _usesExternalZkClient = false; // false by default
public StrictMatchExternalViewVerifier build() {
if (_clusterName == null) {
@@ -102,7 +112,8 @@ public class StrictMatchExternalViewVerifier extends ZkHelixClusterVerifier {
if (_zkClient != null) {
return new StrictMatchExternalViewVerifier(_zkClient, _clusterName, _resources,
- _expectLiveInstances, _isDeactivatedNodeAware, _waitPeriodTillVerify);
+ _expectLiveInstances, _isDeactivatedNodeAware, _waitPeriodTillVerify,
+ _usesExternalZkClient);
}
if (_realmAwareZkConnectionConfig == null || _realmAwareZkClientConfig == null) {
@@ -115,7 +126,8 @@ public class StrictMatchExternalViewVerifier extends ZkHelixClusterVerifier {
return new StrictMatchExternalViewVerifier(
createZkClient(RealmAwareZkClient.RealmMode.SINGLE_REALM, _realmAwareZkConnectionConfig,
_realmAwareZkClientConfig, _zkAddress), _clusterName, _resources,
- _expectLiveInstances, _isDeactivatedNodeAware, _waitPeriodTillVerify);
+ _expectLiveInstances, _isDeactivatedNodeAware, _waitPeriodTillVerify,
+ _usesExternalZkClient);
}
public Builder(String clusterName) {
@@ -151,6 +163,7 @@ public class StrictMatchExternalViewVerifier extends ZkHelixClusterVerifier {
@Deprecated
public Builder setZkClient(RealmAwareZkClient zkClient) {
_zkClient = zkClient;
+ _usesExternalZkClient = true; // Set the flag since external ZkClient is used
return this;
}
diff --git a/helix-core/src/main/java/org/apache/helix/tools/ClusterVerifiers/ZkHelixClusterVerifier.java b/helix-core/src/main/java/org/apache/helix/tools/ClusterVerifiers/ZkHelixClusterVerifier.java
index 9050592..623a91e 100644
--- a/helix-core/src/main/java/org/apache/helix/tools/ClusterVerifiers/ZkHelixClusterVerifier.java
+++ b/helix-core/src/main/java/org/apache/helix/tools/ClusterVerifiers/ZkHelixClusterVerifier.java
@@ -94,12 +94,13 @@ public abstract class ZkHelixClusterVerifier
}
}
- protected ZkHelixClusterVerifier(RealmAwareZkClient zkClient, String clusterName, int waitPeriodTillVerify) {
+ protected ZkHelixClusterVerifier(RealmAwareZkClient zkClient, String clusterName,
+ boolean usesExternalZkClient, int waitPeriodTillVerify) {
if (zkClient == null || clusterName == null) {
throw new IllegalArgumentException("requires zkClient|clusterName");
}
_zkClient = zkClient;
- _usesExternalZkClient = true;
+ _usesExternalZkClient = usesExternalZkClient;
_clusterName = clusterName;
_accessor = new ZKHelixDataAccessor(clusterName, new ZkBaseDataAccessor<>(_zkClient));
_keyBuilder = _accessor.keyBuilder();
@@ -113,6 +114,9 @@ public abstract class ZkHelixClusterVerifier
}
// If the multi ZK config is enabled, use DedicatedZkClient on multi-realm mode
if (Boolean.getBoolean(SystemPropertyKeys.MULTI_ZK_ENABLED) || zkAddr == null) {
+ LOG.info(
+ "ZkHelixClusterVerifier: zkAddr is null or multi-zk mode is enabled in System Properties."
+ + " Instantiating in multi-zk mode!");
try {
RealmAwareZkClient.RealmAwareZkConnectionConfig.Builder connectionConfigBuilder =
new RealmAwareZkClient.RealmAwareZkConnectionConfig.Builder();
@@ -127,9 +131,6 @@ public abstract class ZkHelixClusterVerifier
throw new HelixException("ZkHelixClusterVerifier: failed to create ZkClient!", e);
}
} else {
- if (zkAddr == null) {
- throw new IllegalArgumentException("ZkHelixClusterVerifier: ZkAddress is null or empty!");
- }
_zkClient = DedicatedZkClientFactory.getInstance()
.buildZkClient(new HelixZkClient.ZkConnectionConfig(zkAddr));
}