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 <[email protected]>
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));
}
