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/09/03 19:49:24 UTC
[GitHub] [helix] jiajunwang commented on a change in pull request #1328: Implement TestBestPossibleExternalViewVerifier
jiajunwang commented on a change in pull request #1328:
URL: https://github.com/apache/helix/pull/1328#discussion_r483206728
##########
File path: helix-core/src/main/java/org/apache/helix/tools/ClusterVerifiers/TestBestPossibleExternalViewVerifier.java
##########
@@ -0,0 +1,133 @@
+package org.apache.helix.tools.ClusterVerifiers;
+
+import java.util.Map;
+import java.util.Set;
+
+import org.apache.helix.zookeeper.api.client.RealmAwareZkClient;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+
+public class TestBestPossibleExternalViewVerifier extends BestPossibleExternalViewVerifier {
+ private static Logger LOG = LoggerFactory.getLogger(TestBestPossibleExternalViewVerifier.class);
+ private static int COOL_DOWN = 2 * 1000;
+
+ private TestBestPossibleExternalViewVerifier(RealmAwareZkClient zkClient, String clusterName,
+ Map<String, Map<String, String>> errStates, Set<String> resources,
+ Set<String> expectLiveInstances) {
+ super (zkClient, clusterName, errStates, resources, expectLiveInstances);
+ }
+ /**
+ * Deprecated - please use the Builder to construct this class.
+ * @param zkAddr
+ * @param clusterName
+ * @param resources
+ * @param errStates
+ * @param expectLiveInstances
+ */
+ @Deprecated
Review comment:
Why are we adding new "Deprecated" methods?
##########
File path: helix-core/src/main/java/org/apache/helix/tools/ClusterVerifiers/TestBestPossibleExternalViewVerifier.java
##########
@@ -0,0 +1,133 @@
+package org.apache.helix.tools.ClusterVerifiers;
+
+import java.util.Map;
+import java.util.Set;
+
+import org.apache.helix.zookeeper.api.client.RealmAwareZkClient;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+
+public class TestBestPossibleExternalViewVerifier extends BestPossibleExternalViewVerifier {
+ private static Logger LOG = LoggerFactory.getLogger(TestBestPossibleExternalViewVerifier.class);
+ private static int COOL_DOWN = 2 * 1000;
+
+ private TestBestPossibleExternalViewVerifier(RealmAwareZkClient zkClient, String clusterName,
+ Map<String, Map<String, String>> errStates, Set<String> resources,
+ Set<String> expectLiveInstances) {
+ super (zkClient, clusterName, errStates, resources, expectLiveInstances);
+ }
+ /**
+ * Deprecated - please use the Builder to construct this class.
+ * @param zkAddr
+ * @param clusterName
+ * @param resources
+ * @param errStates
+ * @param expectLiveInstances
+ */
+ @Deprecated
+ public TestBestPossibleExternalViewVerifier(String zkAddr, String clusterName, Set<String> resources,
+ Map<String, Map<String, String>> errStates, Set<String> expectLiveInstances) {
+ super(zkAddr, clusterName, resources, errStates, expectLiveInstances);
+ }
+
+ /**
+ * Deprecated - please use the Builder to construct this class.
+ * @param zkClient
+ * @param clusterName
+ * @param resources
+ * @param errStates
+ * @param expectLiveInstances
+ */
+ @Deprecated
+ public TestBestPossibleExternalViewVerifier(RealmAwareZkClient zkClient, String clusterName,
+ Set<String> resources, Map<String, Map<String, String>> errStates,
+ Set<String> expectLiveInstances) {
+ super(zkClient, clusterName, resources, errStates, expectLiveInstances);
+ }
+
+ public static class Builder extends ZkHelixClusterVerifier.Builder<TestBestPossibleExternalViewVerifier.Builder> {
+ private final String _clusterName;
+ private Map<String, Map<String, String>> _errStates;
+ private Set<String> _resources;
+ private Set<String> _expectLiveInstances;
+ private RealmAwareZkClient _zkClient;
+
+ public Builder(String clusterName) {
+ _clusterName = clusterName;
+ }
+
+ public TestBestPossibleExternalViewVerifier build() {
+ if (_clusterName == null) {
+ throw new IllegalArgumentException("Cluster name is missing!");
+ }
+
+ if (_zkClient != null) {
+ return new TestBestPossibleExternalViewVerifier(_zkClient, _clusterName, _resources, _errStates,
+ _expectLiveInstances);
+ }
+
+ if (_realmAwareZkConnectionConfig == null || _realmAwareZkClientConfig == null) {
+ // For backward-compatibility
+ return new TestBestPossibleExternalViewVerifier(_zkAddress, _clusterName, _resources,
+ _errStates, _expectLiveInstances);
+ }
+
+ validate();
+ return new TestBestPossibleExternalViewVerifier(
+ createZkClient(RealmAwareZkClient.RealmMode.SINGLE_REALM, _realmAwareZkConnectionConfig,
+ _realmAwareZkClientConfig, _zkAddress), _clusterName, _errStates, _resources,
+ _expectLiveInstances);
+ }
+
+ public String getClusterName() {
+ return _clusterName;
+ }
+
+ public Map<String, Map<String, String>> getErrStates() {
+ return _errStates;
+ }
+
+ public TestBestPossibleExternalViewVerifier.Builder setErrStates(Map<String, Map<String, String>> errStates) {
+ _errStates = errStates;
+ return this;
+ }
+
+ public Set<String> getResources() {
+ return _resources;
+ }
+
+ public TestBestPossibleExternalViewVerifier.Builder setResources(Set<String> resources) {
+ _resources = resources;
+ return this;
+ }
+
+ public Set<String> getExpectLiveInstances() {
+ return _expectLiveInstances;
+ }
+
+ public TestBestPossibleExternalViewVerifier.Builder setExpectLiveInstances(Set<String> expectLiveInstances) {
+ _expectLiveInstances = expectLiveInstances;
+ return this;
+ }
+
+ public String getZkAddr() {
+ return _zkAddress;
+ }
+
+ public TestBestPossibleExternalViewVerifier.Builder setZkClient(RealmAwareZkClient zkClient) {
+ _zkClient = zkClient;
+ return this;
+ }
+ }
+
+ @Override
+ public boolean verifyByPolling(long timeout, long period) {
Review comment:
There are more verify methods, I think we shall add COOL_DOWN to all of them. Otherwise, this verifier will behave inconsistently.
##########
File path: helix-core/src/main/java/org/apache/helix/tools/ClusterVerifiers/TestBestPossibleExternalViewVerifier.java
##########
@@ -0,0 +1,133 @@
+package org.apache.helix.tools.ClusterVerifiers;
+
+import java.util.Map;
+import java.util.Set;
+
+import org.apache.helix.zookeeper.api.client.RealmAwareZkClient;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+
+public class TestBestPossibleExternalViewVerifier extends BestPossibleExternalViewVerifier {
Review comment:
TestBestPossibleExternalViewVerifier => BestPossibleExternalViewVerifierWithCoolDown?
##########
File path: helix-core/src/main/java/org/apache/helix/tools/ClusterVerifiers/TestBestPossibleExternalViewVerifier.java
##########
@@ -0,0 +1,133 @@
+package org.apache.helix.tools.ClusterVerifiers;
+
+import java.util.Map;
+import java.util.Set;
+
+import org.apache.helix.zookeeper.api.client.RealmAwareZkClient;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+
+public class TestBestPossibleExternalViewVerifier extends BestPossibleExternalViewVerifier {
+ private static Logger LOG = LoggerFactory.getLogger(TestBestPossibleExternalViewVerifier.class);
+ private static int COOL_DOWN = 2 * 1000;
Review comment:
Make it configurable?
##########
File path: helix-core/src/main/java/org/apache/helix/tools/ClusterVerifiers/TestBestPossibleExternalViewVerifier.java
##########
@@ -0,0 +1,133 @@
+package org.apache.helix.tools.ClusterVerifiers;
+
+import java.util.Map;
+import java.util.Set;
+
+import org.apache.helix.zookeeper.api.client.RealmAwareZkClient;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+
+public class TestBestPossibleExternalViewVerifier extends BestPossibleExternalViewVerifier {
+ private static Logger LOG = LoggerFactory.getLogger(TestBestPossibleExternalViewVerifier.class);
+ private static int COOL_DOWN = 2 * 1000;
+
+ private TestBestPossibleExternalViewVerifier(RealmAwareZkClient zkClient, String clusterName,
+ Map<String, Map<String, String>> errStates, Set<String> resources,
+ Set<String> expectLiveInstances) {
+ super (zkClient, clusterName, errStates, resources, expectLiveInstances);
+ }
+ /**
+ * Deprecated - please use the Builder to construct this class.
+ * @param zkAddr
+ * @param clusterName
+ * @param resources
+ * @param errStates
+ * @param expectLiveInstances
+ */
+ @Deprecated
+ public TestBestPossibleExternalViewVerifier(String zkAddr, String clusterName, Set<String> resources,
+ Map<String, Map<String, String>> errStates, Set<String> expectLiveInstances) {
+ super(zkAddr, clusterName, resources, errStates, expectLiveInstances);
+ }
+
+ /**
+ * Deprecated - please use the Builder to construct this class.
+ * @param zkClient
+ * @param clusterName
+ * @param resources
+ * @param errStates
+ * @param expectLiveInstances
+ */
+ @Deprecated
+ public TestBestPossibleExternalViewVerifier(RealmAwareZkClient zkClient, String clusterName,
+ Set<String> resources, Map<String, Map<String, String>> errStates,
+ Set<String> expectLiveInstances) {
+ super(zkClient, clusterName, resources, errStates, expectLiveInstances);
+ }
+
+ public static class Builder extends ZkHelixClusterVerifier.Builder<TestBestPossibleExternalViewVerifier.Builder> {
Review comment:
I had a quick try, I think will work fine. You don't need to copy-paste all code.
public static class Builder extends BestPossibleExternalViewVerifier.Builder {
public Builder(String clusterName) {
super(clusterName);
}
}
##########
File path: helix-core/src/main/java/org/apache/helix/tools/ClusterVerifiers/TestBestPossibleExternalViewVerifier.java
##########
@@ -0,0 +1,133 @@
+package org.apache.helix.tools.ClusterVerifiers;
+
+import java.util.Map;
+import java.util.Set;
+
+import org.apache.helix.zookeeper.api.client.RealmAwareZkClient;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+
+public class TestBestPossibleExternalViewVerifier extends BestPossibleExternalViewVerifier {
Review comment:
License and a comment for the class, please.
##########
File path: helix-core/src/main/java/org/apache/helix/tools/ClusterVerifiers/ZkHelixClusterVerifier.java
##########
@@ -234,10 +237,14 @@ protected boolean verifyByCallback(long timeout, List<ClusterVerifyTrigger> trig
if (!success) {
// make a final try if timeout
success = verifyState();
+ if (!success) {
+ LOG.error("verifyByCallback failed due to timeout, with stack trace {}",
+ Arrays.asList(Thread.currentThread().getStackTrace()));
Review comment:
Stack trace shall be debug log.
##########
File path: helix-core/src/main/java/org/apache/helix/tools/ClusterVerifiers/TestBestPossibleExternalViewVerifier.java
##########
@@ -0,0 +1,133 @@
+package org.apache.helix.tools.ClusterVerifiers;
+
+import java.util.Map;
+import java.util.Set;
+
+import org.apache.helix.zookeeper.api.client.RealmAwareZkClient;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+
+public class TestBestPossibleExternalViewVerifier extends BestPossibleExternalViewVerifier {
Review comment:
Moreover, my suggestion is that we put it to the test paths, not in the main.
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org