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/08/27 23:56:03 UTC
[GitHub] [helix] kaisun2000 opened a new pull request #1328: Implement TestBestPossibleExternalViewVerifier #1321
kaisun2000 opened a new pull request #1328:
URL: https://github.com/apache/helix/pull/1328
### Issues
- [x] My PR addresses the following Helix issues and references them in the PR description:
Resolve #1321
### Description
- [x] Here are some details about my PR, including screenshots of any UI changes:
TestBestPossibleExternalViewVerifier would have a default COOL_DOWN period
before verifyByPolling. Due to #526, currently we have randow sleep in the
test before verifyByPolling. We intend to use TestBestPossibeVerifier
instead in our unit test. While keep using BestPossibleExternalViewVerifier
in production code.
### Tests
- [x] The following tests are written for this issue:
None
- [x] The following is the result of the "mvn test" command on the appropriate module:
Running
### Documentation (Optional)
- In case of new functionality, my PR adds documentation in the following wiki page:
(Link the GitHub wiki you added)
### Commits
- My commits all reference appropriate Apache Helix GitHub issues in their subject lines. In addition, my commits follow the guidelines from "[How to write a good git commit message](http://chris.beams.io/posts/git-commit/)":
1. Subject is separated from body by a blank line
1. Subject is limited to 50 characters (not including Jira issue reference)
1. Subject does not end with a period
1. Subject uses the imperative mood ("add", not "adding")
1. Body wraps at 72 characters
1. Body explains "what" and "why", not "how"
### Code Quality
- My diff has been formatted using helix-style.xml
(helix-style-intellij.xml if IntelliJ IDE is used)
----------------------------------------------------------------
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
[GitHub] [helix] jiajunwang commented on pull request #1328: Implement TestBestPossibleExternalViewVerifier
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on pull request #1328:
URL: https://github.com/apache/helix/pull/1328#issuecomment-691374998
----------------------------------------------------------------
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
[GitHub] [helix] kaisun2000 commented on a change in pull request #1328: Implement TestBestPossibleExternalViewVerifier
Posted by GitBox <gi...@apache.org>.
kaisun2000 commented on a change in pull request #1328:
URL: https://github.com/apache/helix/pull/1328#discussion_r486738844
##########
File path: helix-core/src/main/java/org/apache/helix/tools/ClusterVerifiers/BestPossibleExternalViewVerifier.java
##########
@@ -105,7 +105,7 @@ public BestPossibleExternalViewVerifier(RealmAwareZkClient zkClient, String clus
_dataProvider = new ResourceControllerDataProvider();
}
- private BestPossibleExternalViewVerifier(RealmAwareZkClient zkClient, String clusterName,
+ protected BestPossibleExternalViewVerifier(RealmAwareZkClient zkClient, String clusterName,
Review comment:
refactored.
----------------------------------------------------------------
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
[GitHub] [helix] kaisun2000 commented on a change in pull request #1328: Implement TestBestPossibleExternalViewVerifier
Posted by GitBox <gi...@apache.org>.
kaisun2000 commented on a change in pull request #1328:
URL: https://github.com/apache/helix/pull/1328#discussion_r486677357
##########
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:
I think we don't need to change verifyByZkCallback. To me, it won't help. What is your take?
----------------------------------------------------------------
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
[GitHub] [helix] jiajunwang commented on pull request #1328: Implement TestBestPossibleExternalViewVerifier
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on pull request #1328:
URL: https://github.com/apache/helix/pull/1328#issuecomment-691374998
----------------------------------------------------------------
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
[GitHub] [helix] kaisun2000 commented on a change in pull request #1328: Implement TestBestPossibleExternalViewVerifier
Posted by GitBox <gi...@apache.org>.
kaisun2000 commented on a change in pull request #1328:
URL: https://github.com/apache/helix/pull/1328#discussion_r487284222
##########
File path: helix-core/src/main/java/org/apache/helix/tools/ClusterVerifiers/ZkHelixClusterVerifier.java
##########
@@ -335,6 +352,11 @@ public B setZkAddr(String zkAddress) {
return setZkAddress(zkAddress);
}
+ public B setWaitTillVerify(int waitPeriod) {
+ _coolDown = waitPeriod;
Review comment:
changed
----------------------------------------------------------------
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
[GitHub] [helix] kaisun2000 commented on a change in pull request #1328: Implement TestBestPossibleExternalViewVerifier
Posted by GitBox <gi...@apache.org>.
kaisun2000 commented on a change in pull request #1328:
URL: https://github.com/apache/helix/pull/1328#discussion_r487264802
##########
File path: helix-core/src/main/java/org/apache/helix/tools/ClusterVerifiers/BestPossibleExternalViewVerifier.java
##########
@@ -224,12 +224,15 @@ public boolean verifyByZkCallback(long timeout) {
@Override
protected synchronized boolean verifyState() {
try {
+ LOG.debug("Verifier start verifyState at {}", System.currentTimeMillis());
Review comment:
removed all
----------------------------------------------------------------
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
[GitHub] [helix] kaisun2000 commented on a change in pull request #1328: Implement TestBestPossibleExternalViewVerifier
Posted by GitBox <gi...@apache.org>.
kaisun2000 commented on a change in pull request #1328:
URL: https://github.com/apache/helix/pull/1328#discussion_r486738697
##########
File path: helix-core/src/main/java/org/apache/helix/tools/ClusterVerifiers/ZkHelixClusterVerifier.java
##########
@@ -19,6 +19,7 @@
* under the License.
*/
+import java.util.Arrays;
Review comment:
removed.
##########
File path: helix-core/src/main/java/org/apache/helix/tools/ClusterVerifiers/ZkHelixClusterVerifier.java
##########
@@ -234,6 +236,9 @@ 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");
Review comment:
fixed.
##########
File path: helix-core/src/main/java/org/apache/helix/tools/ClusterVerifiers/BestPossibleExternalViewVerifier.java
##########
@@ -105,7 +105,7 @@ public BestPossibleExternalViewVerifier(RealmAwareZkClient zkClient, String clus
_dataProvider = new ResourceControllerDataProvider();
}
- private BestPossibleExternalViewVerifier(RealmAwareZkClient zkClient, String clusterName,
+ protected BestPossibleExternalViewVerifier(RealmAwareZkClient zkClient, String clusterName,
Review comment:
refactored.
##########
File path: helix-core/src/main/java/org/apache/helix/tools/ClusterVerifiers/BestPossibleExternalViewVerifier.java
##########
@@ -224,12 +224,15 @@ public boolean verifyByZkCallback(long timeout) {
@Override
protected synchronized boolean verifyState() {
try {
+ LOG.debug("Verifier start verifyState at {}", System.currentTimeMillis());
Review comment:
removed all
##########
File path: helix-core/src/main/java/org/apache/helix/tools/ClusterVerifiers/ZkHelixClusterVerifier.java
##########
@@ -335,6 +352,11 @@ public B setZkAddr(String zkAddress) {
return setZkAddress(zkAddress);
}
+ public B setWaitTillVerify(int waitPeriod) {
+ _coolDown = waitPeriod;
Review comment:
changed
##########
File path: helix-core/src/main/java/org/apache/helix/tools/ClusterVerifiers/ZkHelixClusterVerifier.java
##########
@@ -187,6 +190,14 @@ public boolean verifyByZkCallback() {
* @return
*/
public boolean verifyByPolling(long timeout, long period) {
Review comment:
make sense.
##########
File path: helix-core/src/main/java/org/apache/helix/tools/ClusterVerifiers/ZkHelixClusterVerifier.java
##########
@@ -19,6 +19,7 @@
* under the License.
*/
+import java.util.Arrays;
Review comment:
removed.
##########
File path: helix-core/src/main/java/org/apache/helix/tools/ClusterVerifiers/ZkHelixClusterVerifier.java
##########
@@ -234,6 +236,9 @@ 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");
Review comment:
fixed.
##########
File path: helix-core/src/main/java/org/apache/helix/tools/ClusterVerifiers/BestPossibleExternalViewVerifier.java
##########
@@ -105,7 +105,7 @@ public BestPossibleExternalViewVerifier(RealmAwareZkClient zkClient, String clus
_dataProvider = new ResourceControllerDataProvider();
}
- private BestPossibleExternalViewVerifier(RealmAwareZkClient zkClient, String clusterName,
+ protected BestPossibleExternalViewVerifier(RealmAwareZkClient zkClient, String clusterName,
Review comment:
refactored.
##########
File path: helix-core/src/main/java/org/apache/helix/tools/ClusterVerifiers/BestPossibleExternalViewVerifier.java
##########
@@ -224,12 +224,15 @@ public boolean verifyByZkCallback(long timeout) {
@Override
protected synchronized boolean verifyState() {
try {
+ LOG.debug("Verifier start verifyState at {}", System.currentTimeMillis());
Review comment:
removed all
##########
File path: helix-core/src/main/java/org/apache/helix/tools/ClusterVerifiers/ZkHelixClusterVerifier.java
##########
@@ -335,6 +352,11 @@ public B setZkAddr(String zkAddress) {
return setZkAddress(zkAddress);
}
+ public B setWaitTillVerify(int waitPeriod) {
+ _coolDown = waitPeriod;
Review comment:
changed
##########
File path: helix-core/src/main/java/org/apache/helix/tools/ClusterVerifiers/ZkHelixClusterVerifier.java
##########
@@ -187,6 +190,14 @@ public boolean verifyByZkCallback() {
* @return
*/
public boolean verifyByPolling(long timeout, long period) {
Review comment:
make sense.
----------------------------------------------------------------
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
[GitHub] [helix] lei-xia commented on a change in pull request #1328: Implement TestBestPossibleExternalViewVerifier
Posted by GitBox <gi...@apache.org>.
lei-xia commented on a change in pull request #1328:
URL: https://github.com/apache/helix/pull/1328#discussion_r479480408
##########
File path: helix-core/src/main/java/org/apache/helix/tools/ClusterVerifiers/BestPossibleExternalViewVerifier.java
##########
@@ -245,13 +248,17 @@ protected synchronized boolean verifyState() {
return false;
}
}
+
+ LOG.debug("Verifier finished live instances at {}", System.currentTimeMillis());
Review comment:
"finished retrieving live instances"
----------------------------------------------------------------
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
[GitHub] [helix] kaisun2000 commented on a change in pull request #1328: Implement TestBestPossibleExternalViewVerifier
Posted by GitBox <gi...@apache.org>.
kaisun2000 commented on a change in pull request #1328:
URL: https://github.com/apache/helix/pull/1328#discussion_r485989056
##########
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:
done.
----------------------------------------------------------------
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
[GitHub] [helix] kaisun2000 commented on a change in pull request #1328: Implement TestBestPossibleExternalViewVerifier
Posted by GitBox <gi...@apache.org>.
kaisun2000 commented on a change in pull request #1328:
URL: https://github.com/apache/helix/pull/1328#discussion_r485989274
##########
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:
changed the path and name. added license,
----------------------------------------------------------------
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
[GitHub] [helix] kaisun2000 commented on a change in pull request #1328: Implement TestBestPossibleExternalViewVerifier
Posted by GitBox <gi...@apache.org>.
kaisun2000 commented on a change in pull request #1328:
URL: https://github.com/apache/helix/pull/1328#discussion_r486738697
##########
File path: helix-core/src/main/java/org/apache/helix/tools/ClusterVerifiers/ZkHelixClusterVerifier.java
##########
@@ -19,6 +19,7 @@
* under the License.
*/
+import java.util.Arrays;
Review comment:
removed.
##########
File path: helix-core/src/main/java/org/apache/helix/tools/ClusterVerifiers/ZkHelixClusterVerifier.java
##########
@@ -234,6 +236,9 @@ 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");
Review comment:
fixed.
----------------------------------------------------------------
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
[GitHub] [helix] jiajunwang commented on a change in pull request #1328: Implement TestBestPossibleExternalViewVerifier
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #1328:
URL: https://github.com/apache/helix/pull/1328#discussion_r486000929
##########
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:
But you didn't change "verifyByZkCallback", right?
----------------------------------------------------------------
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
[GitHub] [helix] kaisun2000 commented on pull request #1328: Implement TestBestPossibleExternalViewVerifier
Posted by GitBox <gi...@apache.org>.
kaisun2000 commented on pull request #1328:
URL: https://github.com/apache/helix/pull/1328#issuecomment-691311823
This diff is approve, please help to merge into trunk.
>TestBestPossibleExternalViewVerifier would have a default COOL_DOWN period
before verifyByPolling. Due to #526, currently we have randow sleep in the
test before verifyByPolling. We intend to use TestBestPossibeVerifier
instead in our unit test. While keep using BestPossibleExternalViewVerifier
in production code.
----------------------------------------------------------------
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
[GitHub] [helix] jiajunwang commented on a change in pull request #1328: Implement TestBestPossibleExternalViewVerifier
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #1328:
URL: https://github.com/apache/helix/pull/1328#discussion_r486000119
##########
File path: helix-core/src/main/java/org/apache/helix/tools/ClusterVerifiers/ZkHelixClusterVerifier.java
##########
@@ -234,6 +236,9 @@ 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");
Review comment:
nit, keep the same log string style?
"LOG.error("verifier timeout out with timeout {}", timeout);"
##########
File path: helix-core/src/main/java/org/apache/helix/tools/ClusterVerifiers/BestPossibleExternalViewVerifier.java
##########
@@ -105,7 +105,7 @@ public BestPossibleExternalViewVerifier(RealmAwareZkClient zkClient, String clus
_dataProvider = new ResourceControllerDataProvider();
}
- private BestPossibleExternalViewVerifier(RealmAwareZkClient zkClient, String clusterName,
+ protected BestPossibleExternalViewVerifier(RealmAwareZkClient zkClient, String clusterName,
Review comment:
This breaks the builder design pattern a little bit.
I'm thinking if we have a better way.
What if we add a configuration to the Verifier Builder called "waitUntilVerify"? It can be a generic configuration for all verifiers even production ones.
1. It is optional, so no backward compatibility issue.
2. It potentially helps our customers too, because they may face the same verify timing issue.
3. It requires the same amount of change as your current design. But no extra class introduced.
Please consider adding it to ZkHelixClusterVerifier.Builder, so all the verifier gets it.
##########
File path: helix-core/src/main/java/org/apache/helix/tools/ClusterVerifiers/BestPossibleExternalViewVerifier.java
##########
@@ -224,12 +224,15 @@ public boolean verifyByZkCallback(long timeout) {
@Override
protected synchronized boolean verifyState() {
try {
+ LOG.debug("Verifier start verifyState at {}", System.currentTimeMillis());
Review comment:
The timestamp seems not to be removed.
And I really don't like these outputs even as debug. I think they are only useful when debugging. And we shall not add them to the repo.
##########
File path: helix-core/src/main/java/org/apache/helix/tools/ClusterVerifiers/ZkHelixClusterVerifier.java
##########
@@ -19,6 +19,7 @@
* under the License.
*/
+import java.util.Arrays;
Review comment:
Remove?
----------------------------------------------------------------
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
[GitHub] [helix] jiajunwang commented on a change in pull request #1328: Implement TestBestPossibleExternalViewVerifier
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #1328:
URL: https://github.com/apache/helix/pull/1328#discussion_r487218755
##########
File path: helix-core/src/main/java/org/apache/helix/tools/ClusterVerifiers/ZkHelixClusterVerifier.java
##########
@@ -335,6 +352,11 @@ public B setZkAddr(String zkAddress) {
return setZkAddress(zkAddress);
}
+ public B setWaitTillVerify(int waitPeriod) {
+ _coolDown = waitPeriod;
Review comment:
Let's just call it "waitPeriodTillVerify. Cooldown is not an obvious name.
##########
File path: helix-core/src/main/java/org/apache/helix/tools/ClusterVerifiers/BestPossibleExternalViewVerifier.java
##########
@@ -224,12 +224,15 @@ public boolean verifyByZkCallback(long timeout) {
@Override
protected synchronized boolean verifyState() {
try {
+ LOG.debug("Verifier start verifyState at {}", System.currentTimeMillis());
Review comment:
As we discussed, please remove the debug logs if not absolutely necessary for long term usage.
##########
File path: helix-core/src/main/java/org/apache/helix/tools/ClusterVerifiers/ZkHelixClusterVerifier.java
##########
@@ -187,6 +190,14 @@ public boolean verifyByZkCallback() {
* @return
*/
public boolean verifyByPolling(long timeout, long period) {
Review comment:
I believe you can and should add the same wait for verifyByZkCallback too.
The logic of verifyByZkCallback is that
1. subscribe to changes.
2. no matter if change happens, verify the state for once.
3. if subscription results in some notification, then verify in parallel.
4. any of the paths succeed, then the verify succeeds.
In this case, we shall wait until the first subscribe to ensure the waitTillVerify config takes effect. This won't cause any problem with verifying logic. Since if the change happens before we subscribe, we are still fine because the first default verify check will be done once at least.
##########
File path: helix-core/src/main/java/org/apache/helix/tools/ClusterVerifiers/ZkHelixClusterVerifier.java
##########
@@ -335,6 +352,11 @@ public B setZkAddr(String zkAddress) {
return setZkAddress(zkAddress);
}
+ public B setWaitTillVerify(int waitPeriod) {
+ _coolDown = waitPeriod;
Review comment:
Let's just call it "waitPeriodTillVerify. Cooldown is not an obvious name.
##########
File path: helix-core/src/main/java/org/apache/helix/tools/ClusterVerifiers/BestPossibleExternalViewVerifier.java
##########
@@ -224,12 +224,15 @@ public boolean verifyByZkCallback(long timeout) {
@Override
protected synchronized boolean verifyState() {
try {
+ LOG.debug("Verifier start verifyState at {}", System.currentTimeMillis());
Review comment:
As we discussed, please remove the debug logs if not absolutely necessary for long term usage.
##########
File path: helix-core/src/main/java/org/apache/helix/tools/ClusterVerifiers/ZkHelixClusterVerifier.java
##########
@@ -187,6 +190,14 @@ public boolean verifyByZkCallback() {
* @return
*/
public boolean verifyByPolling(long timeout, long period) {
Review comment:
I believe you can and should add the same wait for verifyByZkCallback too.
The logic of verifyByZkCallback is that
1. subscribe to changes.
2. no matter if change happens, verify the state for once.
3. if subscription results in some notification, then verify in parallel.
4. any of the paths succeed, then the verify succeeds.
In this case, we shall wait until the first subscribe to ensure the waitTillVerify config takes effect. This won't cause any problem with verifying logic. Since if the change happens before we subscribe, we are still fine because the first default verify check will be done once at least.
##########
File path: helix-core/src/main/java/org/apache/helix/tools/ClusterVerifiers/ZkHelixClusterVerifier.java
##########
@@ -335,6 +352,11 @@ public B setZkAddr(String zkAddress) {
return setZkAddress(zkAddress);
}
+ public B setWaitTillVerify(int waitPeriod) {
+ _coolDown = waitPeriod;
Review comment:
Let's just call it "waitPeriodTillVerify. Cooldown is not an obvious name.
##########
File path: helix-core/src/main/java/org/apache/helix/tools/ClusterVerifiers/BestPossibleExternalViewVerifier.java
##########
@@ -224,12 +224,15 @@ public boolean verifyByZkCallback(long timeout) {
@Override
protected synchronized boolean verifyState() {
try {
+ LOG.debug("Verifier start verifyState at {}", System.currentTimeMillis());
Review comment:
As we discussed, please remove the debug logs if not absolutely necessary for long term usage.
##########
File path: helix-core/src/main/java/org/apache/helix/tools/ClusterVerifiers/ZkHelixClusterVerifier.java
##########
@@ -187,6 +190,14 @@ public boolean verifyByZkCallback() {
* @return
*/
public boolean verifyByPolling(long timeout, long period) {
Review comment:
I believe you can and should add the same wait for verifyByZkCallback too.
The logic of verifyByZkCallback is that
1. subscribe to changes.
2. no matter if change happens, verify the state for once.
3. if subscription results in some notification, then verify in parallel.
4. any of the paths succeed, then the verify succeeds.
In this case, we shall wait until the first subscribe to ensure the waitTillVerify config takes effect. This won't cause any problem with verifying logic. Since if the change happens before we subscribe, we are still fine because the first default verify check will be done once at least.
##########
File path: helix-core/src/main/java/org/apache/helix/tools/ClusterVerifiers/ZkHelixClusterVerifier.java
##########
@@ -335,6 +352,11 @@ public B setZkAddr(String zkAddress) {
return setZkAddress(zkAddress);
}
+ public B setWaitTillVerify(int waitPeriod) {
+ _coolDown = waitPeriod;
Review comment:
Let's just call it "waitPeriodTillVerify. Cooldown is not an obvious name.
##########
File path: helix-core/src/main/java/org/apache/helix/tools/ClusterVerifiers/BestPossibleExternalViewVerifier.java
##########
@@ -224,12 +224,15 @@ public boolean verifyByZkCallback(long timeout) {
@Override
protected synchronized boolean verifyState() {
try {
+ LOG.debug("Verifier start verifyState at {}", System.currentTimeMillis());
Review comment:
As we discussed, please remove the debug logs if not absolutely necessary for long term usage.
##########
File path: helix-core/src/main/java/org/apache/helix/tools/ClusterVerifiers/ZkHelixClusterVerifier.java
##########
@@ -187,6 +190,14 @@ public boolean verifyByZkCallback() {
* @return
*/
public boolean verifyByPolling(long timeout, long period) {
Review comment:
I believe you can and should add the same wait for verifyByZkCallback too.
The logic of verifyByZkCallback is that
1. subscribe to changes.
2. no matter if change happens, verify the state for once.
3. if subscription results in some notification, then verify in parallel.
4. any of the paths succeed, then the verify succeeds.
In this case, we shall wait until the first subscribe to ensure the waitTillVerify config takes effect. This won't cause any problem with verifying logic. Since if the change happens before we subscribe, we are still fine because the first default verify check will be done once at least.
##########
File path: helix-core/src/main/java/org/apache/helix/tools/ClusterVerifiers/ZkHelixClusterVerifier.java
##########
@@ -335,6 +352,11 @@ public B setZkAddr(String zkAddress) {
return setZkAddress(zkAddress);
}
+ public B setWaitTillVerify(int waitPeriod) {
+ _coolDown = waitPeriod;
Review comment:
Let's just call it "waitPeriodTillVerify. Cooldown is not an obvious name.
##########
File path: helix-core/src/main/java/org/apache/helix/tools/ClusterVerifiers/BestPossibleExternalViewVerifier.java
##########
@@ -224,12 +224,15 @@ public boolean verifyByZkCallback(long timeout) {
@Override
protected synchronized boolean verifyState() {
try {
+ LOG.debug("Verifier start verifyState at {}", System.currentTimeMillis());
Review comment:
As we discussed, please remove the debug logs if not absolutely necessary for long term usage.
##########
File path: helix-core/src/main/java/org/apache/helix/tools/ClusterVerifiers/ZkHelixClusterVerifier.java
##########
@@ -187,6 +190,14 @@ public boolean verifyByZkCallback() {
* @return
*/
public boolean verifyByPolling(long timeout, long period) {
Review comment:
I believe you can and should add the same wait for verifyByZkCallback too.
The logic of verifyByZkCallback is that
1. subscribe to changes.
2. no matter if change happens, verify the state for once.
3. if subscription results in some notification, then verify in parallel.
4. any of the paths succeed, then the verify succeeds.
In this case, we shall wait until the first subscribe to ensure the waitTillVerify config takes effect. This won't cause any problem with verifying logic. Since if the change happens before we subscribe, we are still fine because the first default verify check will be done once at least.
##########
File path: helix-core/src/main/java/org/apache/helix/tools/ClusterVerifiers/ZkHelixClusterVerifier.java
##########
@@ -335,6 +352,11 @@ public B setZkAddr(String zkAddress) {
return setZkAddress(zkAddress);
}
+ public B setWaitTillVerify(int waitPeriod) {
+ _coolDown = waitPeriod;
Review comment:
Let's just call it "waitPeriodTillVerify. Cooldown is not an obvious name.
##########
File path: helix-core/src/main/java/org/apache/helix/tools/ClusterVerifiers/BestPossibleExternalViewVerifier.java
##########
@@ -224,12 +224,15 @@ public boolean verifyByZkCallback(long timeout) {
@Override
protected synchronized boolean verifyState() {
try {
+ LOG.debug("Verifier start verifyState at {}", System.currentTimeMillis());
Review comment:
As we discussed, please remove the debug logs if not absolutely necessary for long term usage.
##########
File path: helix-core/src/main/java/org/apache/helix/tools/ClusterVerifiers/ZkHelixClusterVerifier.java
##########
@@ -187,6 +190,14 @@ public boolean verifyByZkCallback() {
* @return
*/
public boolean verifyByPolling(long timeout, long period) {
Review comment:
I believe you can and should add the same wait for verifyByZkCallback too.
The logic of verifyByZkCallback is that
1. subscribe to changes.
2. no matter if change happens, verify the state for once.
3. if subscription results in some notification, then verify in parallel.
4. any of the paths succeed, then the verify succeeds.
In this case, we shall wait until the first subscribe to ensure the waitTillVerify config takes effect. This won't cause any problem with verifying logic. Since if the change happens before we subscribe, we are still fine because the first default verify check will be done once at least.
##########
File path: helix-core/src/main/java/org/apache/helix/tools/ClusterVerifiers/ZkHelixClusterVerifier.java
##########
@@ -335,6 +352,11 @@ public B setZkAddr(String zkAddress) {
return setZkAddress(zkAddress);
}
+ public B setWaitTillVerify(int waitPeriod) {
+ _coolDown = waitPeriod;
Review comment:
Let's just call it "waitPeriodTillVerify. Cooldown is not an obvious name.
##########
File path: helix-core/src/main/java/org/apache/helix/tools/ClusterVerifiers/BestPossibleExternalViewVerifier.java
##########
@@ -224,12 +224,15 @@ public boolean verifyByZkCallback(long timeout) {
@Override
protected synchronized boolean verifyState() {
try {
+ LOG.debug("Verifier start verifyState at {}", System.currentTimeMillis());
Review comment:
As we discussed, please remove the debug logs if not absolutely necessary for long term usage.
##########
File path: helix-core/src/main/java/org/apache/helix/tools/ClusterVerifiers/ZkHelixClusterVerifier.java
##########
@@ -187,6 +190,14 @@ public boolean verifyByZkCallback() {
* @return
*/
public boolean verifyByPolling(long timeout, long period) {
Review comment:
I believe you can and should add the same wait for verifyByZkCallback too.
The logic of verifyByZkCallback is that
1. subscribe to changes.
2. no matter if change happens, verify the state for once.
3. if subscription results in some notification, then verify in parallel.
4. any of the paths succeed, then the verify succeeds.
In this case, we shall wait until the first subscribe to ensure the waitTillVerify config takes effect. This won't cause any problem with verifying logic. Since if the change happens before we subscribe, we are still fine because the first default verify check will be done once at least.
----------------------------------------------------------------
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
[GitHub] [helix] kaisun2000 commented on a change in pull request #1328: Implement TestBestPossibleExternalViewVerifier
Posted by GitBox <gi...@apache.org>.
kaisun2000 commented on a change in pull request #1328:
URL: https://github.com/apache/helix/pull/1328#discussion_r487264802
##########
File path: helix-core/src/main/java/org/apache/helix/tools/ClusterVerifiers/BestPossibleExternalViewVerifier.java
##########
@@ -224,12 +224,15 @@ public boolean verifyByZkCallback(long timeout) {
@Override
protected synchronized boolean verifyState() {
try {
+ LOG.debug("Verifier start verifyState at {}", System.currentTimeMillis());
Review comment:
removed all
##########
File path: helix-core/src/main/java/org/apache/helix/tools/ClusterVerifiers/ZkHelixClusterVerifier.java
##########
@@ -335,6 +352,11 @@ public B setZkAddr(String zkAddress) {
return setZkAddress(zkAddress);
}
+ public B setWaitTillVerify(int waitPeriod) {
+ _coolDown = waitPeriod;
Review comment:
changed
##########
File path: helix-core/src/main/java/org/apache/helix/tools/ClusterVerifiers/ZkHelixClusterVerifier.java
##########
@@ -187,6 +190,14 @@ public boolean verifyByZkCallback() {
* @return
*/
public boolean verifyByPolling(long timeout, long period) {
Review comment:
make sense.
##########
File path: helix-core/src/main/java/org/apache/helix/tools/ClusterVerifiers/BestPossibleExternalViewVerifier.java
##########
@@ -224,12 +224,15 @@ public boolean verifyByZkCallback(long timeout) {
@Override
protected synchronized boolean verifyState() {
try {
+ LOG.debug("Verifier start verifyState at {}", System.currentTimeMillis());
Review comment:
removed all
##########
File path: helix-core/src/main/java/org/apache/helix/tools/ClusterVerifiers/ZkHelixClusterVerifier.java
##########
@@ -335,6 +352,11 @@ public B setZkAddr(String zkAddress) {
return setZkAddress(zkAddress);
}
+ public B setWaitTillVerify(int waitPeriod) {
+ _coolDown = waitPeriod;
Review comment:
changed
##########
File path: helix-core/src/main/java/org/apache/helix/tools/ClusterVerifiers/ZkHelixClusterVerifier.java
##########
@@ -187,6 +190,14 @@ public boolean verifyByZkCallback() {
* @return
*/
public boolean verifyByPolling(long timeout, long period) {
Review comment:
make sense.
##########
File path: helix-core/src/main/java/org/apache/helix/tools/ClusterVerifiers/BestPossibleExternalViewVerifier.java
##########
@@ -224,12 +224,15 @@ public boolean verifyByZkCallback(long timeout) {
@Override
protected synchronized boolean verifyState() {
try {
+ LOG.debug("Verifier start verifyState at {}", System.currentTimeMillis());
Review comment:
removed all
##########
File path: helix-core/src/main/java/org/apache/helix/tools/ClusterVerifiers/ZkHelixClusterVerifier.java
##########
@@ -335,6 +352,11 @@ public B setZkAddr(String zkAddress) {
return setZkAddress(zkAddress);
}
+ public B setWaitTillVerify(int waitPeriod) {
+ _coolDown = waitPeriod;
Review comment:
changed
##########
File path: helix-core/src/main/java/org/apache/helix/tools/ClusterVerifiers/ZkHelixClusterVerifier.java
##########
@@ -187,6 +190,14 @@ public boolean verifyByZkCallback() {
* @return
*/
public boolean verifyByPolling(long timeout, long period) {
Review comment:
make sense.
##########
File path: helix-core/src/main/java/org/apache/helix/tools/ClusterVerifiers/BestPossibleExternalViewVerifier.java
##########
@@ -224,12 +224,15 @@ public boolean verifyByZkCallback(long timeout) {
@Override
protected synchronized boolean verifyState() {
try {
+ LOG.debug("Verifier start verifyState at {}", System.currentTimeMillis());
Review comment:
removed all
##########
File path: helix-core/src/main/java/org/apache/helix/tools/ClusterVerifiers/ZkHelixClusterVerifier.java
##########
@@ -335,6 +352,11 @@ public B setZkAddr(String zkAddress) {
return setZkAddress(zkAddress);
}
+ public B setWaitTillVerify(int waitPeriod) {
+ _coolDown = waitPeriod;
Review comment:
changed
##########
File path: helix-core/src/main/java/org/apache/helix/tools/ClusterVerifiers/ZkHelixClusterVerifier.java
##########
@@ -187,6 +190,14 @@ public boolean verifyByZkCallback() {
* @return
*/
public boolean verifyByPolling(long timeout, long period) {
Review comment:
make sense.
##########
File path: helix-core/src/main/java/org/apache/helix/tools/ClusterVerifiers/BestPossibleExternalViewVerifier.java
##########
@@ -224,12 +224,15 @@ public boolean verifyByZkCallback(long timeout) {
@Override
protected synchronized boolean verifyState() {
try {
+ LOG.debug("Verifier start verifyState at {}", System.currentTimeMillis());
Review comment:
removed all
##########
File path: helix-core/src/main/java/org/apache/helix/tools/ClusterVerifiers/ZkHelixClusterVerifier.java
##########
@@ -335,6 +352,11 @@ public B setZkAddr(String zkAddress) {
return setZkAddress(zkAddress);
}
+ public B setWaitTillVerify(int waitPeriod) {
+ _coolDown = waitPeriod;
Review comment:
changed
##########
File path: helix-core/src/main/java/org/apache/helix/tools/ClusterVerifiers/ZkHelixClusterVerifier.java
##########
@@ -187,6 +190,14 @@ public boolean verifyByZkCallback() {
* @return
*/
public boolean verifyByPolling(long timeout, long period) {
Review comment:
make sense.
##########
File path: helix-core/src/main/java/org/apache/helix/tools/ClusterVerifiers/BestPossibleExternalViewVerifier.java
##########
@@ -224,12 +224,15 @@ public boolean verifyByZkCallback(long timeout) {
@Override
protected synchronized boolean verifyState() {
try {
+ LOG.debug("Verifier start verifyState at {}", System.currentTimeMillis());
Review comment:
removed all
##########
File path: helix-core/src/main/java/org/apache/helix/tools/ClusterVerifiers/ZkHelixClusterVerifier.java
##########
@@ -335,6 +352,11 @@ public B setZkAddr(String zkAddress) {
return setZkAddress(zkAddress);
}
+ public B setWaitTillVerify(int waitPeriod) {
+ _coolDown = waitPeriod;
Review comment:
changed
##########
File path: helix-core/src/main/java/org/apache/helix/tools/ClusterVerifiers/ZkHelixClusterVerifier.java
##########
@@ -187,6 +190,14 @@ public boolean verifyByZkCallback() {
* @return
*/
public boolean verifyByPolling(long timeout, long period) {
Review comment:
make sense.
##########
File path: helix-core/src/main/java/org/apache/helix/tools/ClusterVerifiers/BestPossibleExternalViewVerifier.java
##########
@@ -224,12 +224,15 @@ public boolean verifyByZkCallback(long timeout) {
@Override
protected synchronized boolean verifyState() {
try {
+ LOG.debug("Verifier start verifyState at {}", System.currentTimeMillis());
Review comment:
removed all
##########
File path: helix-core/src/main/java/org/apache/helix/tools/ClusterVerifiers/ZkHelixClusterVerifier.java
##########
@@ -335,6 +352,11 @@ public B setZkAddr(String zkAddress) {
return setZkAddress(zkAddress);
}
+ public B setWaitTillVerify(int waitPeriod) {
+ _coolDown = waitPeriod;
Review comment:
changed
##########
File path: helix-core/src/main/java/org/apache/helix/tools/ClusterVerifiers/ZkHelixClusterVerifier.java
##########
@@ -187,6 +190,14 @@ public boolean verifyByZkCallback() {
* @return
*/
public boolean verifyByPolling(long timeout, long period) {
Review comment:
make sense.
----------------------------------------------------------------
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
[GitHub] [helix] jiajunwang commented on a change in pull request #1328: Implement TestBestPossibleExternalViewVerifier
Posted by GitBox <gi...@apache.org>.
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
[GitHub] [helix] kaisun2000 commented on pull request #1328: Enhance ZkHelixVerifier and its related subclass to take a default wait period
Posted by GitBox <gi...@apache.org>.
kaisun2000 commented on pull request #1328:
URL: https://github.com/apache/helix/pull/1328#issuecomment-691380665
This diff is approved, please help to merge.
> Due to #526, currently we have random sleep() in the test before verifyByPolling().
This cause test failure in different environment. Here we enhance ZKHelixVerifier
family with an option in builder to add a waiting period. To keep it backward
compatible, without enable this waiting option, the code would behave exactly the
same as before.
----------------------------------------------------------------
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
[GitHub] [helix] jiajunwang commented on pull request #1328: Implement TestBestPossibleExternalViewVerifier
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on pull request #1328:
URL: https://github.com/apache/helix/pull/1328#issuecomment-691374998
> This diff is approve, please help to merge into trunk.
>
> > TestBestPossibleExternalViewVerifier would have a default COOL_DOWN period
> > before verifyByPolling. Due to #526, currently we have randow sleep in the
> > test before verifyByPolling. We intend to use TestBestPossibeVerifier
> > instead in our unit test. While keep using BestPossibleExternalViewVerifier
> > in production code.
@kaisun2000 please update the comment, it has diverged from the code change quite a lot. Please do take care of the final comment message, once it is in, we don't have a way to modify except reverting the PR.
----------------------------------------------------------------
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
[GitHub] [helix] kaisun2000 commented on pull request #1328: Implement TestBestPossibleExternalViewVerifier
Posted by GitBox <gi...@apache.org>.
kaisun2000 commented on pull request #1328:
URL: https://github.com/apache/helix/pull/1328#issuecomment-689896086
> Many tests (especially WAGED tests) are using StrictMatchExternalViewVerifier. We need to do the same for that too.
>
> Overall, my suggestion is still to add waiting to the specific test case where need it. This generic cooling down logic does not look reasonable to me. The cooldown shall be with the test logic.
>
> One alternative way is that we add a parameter to the verify method that you can specify a cooldown in the caller logic. Could you please consider this solution?
I did a search of places using `verifyByPolling()`. There are between 150 to 200 places.
So shall we have another one called as the following?
```
public boolean verifyByPolling(long timeout, long period, long cooldown) {
```
----------------------------------------------------------------
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
[GitHub] [helix] jiajunwang edited a comment on pull request #1328: Implement TestBestPossibleExternalViewVerifier
Posted by GitBox <gi...@apache.org>.
jiajunwang edited a comment on pull request #1328:
URL: https://github.com/apache/helix/pull/1328#issuecomment-691374998
----------------------------------------------------------------
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
[GitHub] [helix] kaisun2000 commented on a change in pull request #1328: Implement TestBestPossibleExternalViewVerifier
Posted by GitBox <gi...@apache.org>.
kaisun2000 commented on a change in pull request #1328:
URL: https://github.com/apache/helix/pull/1328#discussion_r485979244
##########
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:
```
verify()
verify(long timeout)
verifyByZkCallback()
```
all eventually boils down to `verifyByZkCallback(long timeout)`
`verifyByPolling()` boils down to `verifyByPolling(DEFAULT_TIMEOUT, DEFAULT_PERIOD)`.
So we only need to add cool down here.
----------------------------------------------------------------
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
[GitHub] [helix] kaisun2000 commented on a change in pull request #1328: Implement TestBestPossibleExternalViewVerifier
Posted by GitBox <gi...@apache.org>.
kaisun2000 commented on a change in pull request #1328:
URL: https://github.com/apache/helix/pull/1328#discussion_r480349965
##########
File path: helix-core/src/main/java/org/apache/helix/tools/ClusterVerifiers/BestPossibleExternalViewVerifier.java
##########
@@ -245,13 +248,17 @@ protected synchronized boolean verifyState() {
return false;
}
}
+
+ LOG.debug("Verifier finished live instances at {}", System.currentTimeMillis());
Review comment:
changed.
----------------------------------------------------------------
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
[GitHub] [helix] lei-xia commented on a change in pull request #1328: Implement TestBestPossibleExternalViewVerifier
Posted by GitBox <gi...@apache.org>.
lei-xia commented on a change in pull request #1328:
URL: https://github.com/apache/helix/pull/1328#discussion_r479479754
##########
File path: helix-core/src/main/java/org/apache/helix/tools/ClusterVerifiers/BestPossibleExternalViewVerifier.java
##########
@@ -224,12 +224,15 @@ public boolean verifyByZkCallback(long timeout) {
@Override
protected synchronized boolean verifyState() {
try {
+ LOG.debug("Verifier start verifyState at {}", System.currentTimeMillis());
Review comment:
Do you need System.currentTimeMillis()? Will the log4j print out the timestamp?
----------------------------------------------------------------
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
[GitHub] [helix] kaisun2000 commented on pull request #1328: Implement TestBestPossibleExternalViewVerifier
Posted by GitBox <gi...@apache.org>.
kaisun2000 commented on pull request #1328:
URL: https://github.com/apache/helix/pull/1328#issuecomment-691311823
----------------------------------------------------------------
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
[GitHub] [helix] kaisun2000 commented on pull request #1328: Implement TestBestPossibleExternalViewVerifier
Posted by GitBox <gi...@apache.org>.
kaisun2000 commented on pull request #1328:
URL: https://github.com/apache/helix/pull/1328#issuecomment-691379732
Good point. I did not realize the previous commit message not applying anymore. Let me update it.
----------------------------------------------------------------
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
[GitHub] [helix] jiajunwang commented on a change in pull request #1328: Implement TestBestPossibleExternalViewVerifier
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #1328:
URL: https://github.com/apache/helix/pull/1328#discussion_r487218755
##########
File path: helix-core/src/main/java/org/apache/helix/tools/ClusterVerifiers/ZkHelixClusterVerifier.java
##########
@@ -335,6 +352,11 @@ public B setZkAddr(String zkAddress) {
return setZkAddress(zkAddress);
}
+ public B setWaitTillVerify(int waitPeriod) {
+ _coolDown = waitPeriod;
Review comment:
Let's just call it "waitPeriodTillVerify. Cooldown is not an obvious name.
##########
File path: helix-core/src/main/java/org/apache/helix/tools/ClusterVerifiers/BestPossibleExternalViewVerifier.java
##########
@@ -224,12 +224,15 @@ public boolean verifyByZkCallback(long timeout) {
@Override
protected synchronized boolean verifyState() {
try {
+ LOG.debug("Verifier start verifyState at {}", System.currentTimeMillis());
Review comment:
As we discussed, please remove the debug logs if not absolutely necessary for long term usage.
##########
File path: helix-core/src/main/java/org/apache/helix/tools/ClusterVerifiers/ZkHelixClusterVerifier.java
##########
@@ -187,6 +190,14 @@ public boolean verifyByZkCallback() {
* @return
*/
public boolean verifyByPolling(long timeout, long period) {
Review comment:
I believe you can and should add the same wait for verifyByZkCallback too.
The logic of verifyByZkCallback is that
1. subscribe to changes.
2. no matter if change happens, verify the state for once.
3. if subscription results in some notification, then verify in parallel.
4. any of the paths succeed, then the verify succeeds.
In this case, we shall wait until the first subscribe to ensure the waitTillVerify config takes effect. This won't cause any problem with verifying logic. Since if the change happens before we subscribe, we are still fine because the first default verify check will be done once at least.
----------------------------------------------------------------
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
[GitHub] [helix] kaisun2000 commented on a change in pull request #1328: Implement TestBestPossibleExternalViewVerifier
Posted by GitBox <gi...@apache.org>.
kaisun2000 commented on a change in pull request #1328:
URL: https://github.com/apache/helix/pull/1328#discussion_r483324078
##########
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:
Good.
----------------------------------------------------------------
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
[GitHub] [helix] kaisun2000 commented on a change in pull request #1328: Implement TestBestPossibleExternalViewVerifier
Posted by GitBox <gi...@apache.org>.
kaisun2000 commented on a change in pull request #1328:
URL: https://github.com/apache/helix/pull/1328#discussion_r487284314
##########
File path: helix-core/src/main/java/org/apache/helix/tools/ClusterVerifiers/ZkHelixClusterVerifier.java
##########
@@ -187,6 +190,14 @@ public boolean verifyByZkCallback() {
* @return
*/
public boolean verifyByPolling(long timeout, long period) {
Review comment:
make sense.
----------------------------------------------------------------
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
[GitHub] [helix] kaisun2000 commented on pull request #1328: Implement TestBestPossibleExternalViewVerifier
Posted by GitBox <gi...@apache.org>.
kaisun2000 commented on pull request #1328:
URL: https://github.com/apache/helix/pull/1328#issuecomment-691311823
----------------------------------------------------------------
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
[GitHub] [helix] jiajunwang commented on a change in pull request #1328: Implement TestBestPossibleExternalViewVerifier
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #1328:
URL: https://github.com/apache/helix/pull/1328#discussion_r487218755
##########
File path: helix-core/src/main/java/org/apache/helix/tools/ClusterVerifiers/ZkHelixClusterVerifier.java
##########
@@ -335,6 +352,11 @@ public B setZkAddr(String zkAddress) {
return setZkAddress(zkAddress);
}
+ public B setWaitTillVerify(int waitPeriod) {
+ _coolDown = waitPeriod;
Review comment:
Let's just call it "waitPeriodTillVerify. Cooldown is not an obvious name.
##########
File path: helix-core/src/main/java/org/apache/helix/tools/ClusterVerifiers/BestPossibleExternalViewVerifier.java
##########
@@ -224,12 +224,15 @@ public boolean verifyByZkCallback(long timeout) {
@Override
protected synchronized boolean verifyState() {
try {
+ LOG.debug("Verifier start verifyState at {}", System.currentTimeMillis());
Review comment:
As we discussed, please remove the debug logs if not absolutely necessary for long term usage.
##########
File path: helix-core/src/main/java/org/apache/helix/tools/ClusterVerifiers/ZkHelixClusterVerifier.java
##########
@@ -187,6 +190,14 @@ public boolean verifyByZkCallback() {
* @return
*/
public boolean verifyByPolling(long timeout, long period) {
Review comment:
I believe you can and should add the same wait for verifyByZkCallback too.
The logic of verifyByZkCallback is that
1. subscribe to changes.
2. no matter if change happens, verify the state for once.
3. if subscription results in some notification, then verify in parallel.
4. any of the paths succeed, then the verify succeeds.
In this case, we shall wait until the first subscribe to ensure the waitTillVerify config takes effect. This won't cause any problem with verifying logic. Since if the change happens before we subscribe, we are still fine because the first default verify check will be done once at least.
##########
File path: helix-core/src/main/java/org/apache/helix/tools/ClusterVerifiers/ZkHelixClusterVerifier.java
##########
@@ -335,6 +352,11 @@ public B setZkAddr(String zkAddress) {
return setZkAddress(zkAddress);
}
+ public B setWaitTillVerify(int waitPeriod) {
+ _coolDown = waitPeriod;
Review comment:
Let's just call it "waitPeriodTillVerify. Cooldown is not an obvious name.
##########
File path: helix-core/src/main/java/org/apache/helix/tools/ClusterVerifiers/BestPossibleExternalViewVerifier.java
##########
@@ -224,12 +224,15 @@ public boolean verifyByZkCallback(long timeout) {
@Override
protected synchronized boolean verifyState() {
try {
+ LOG.debug("Verifier start verifyState at {}", System.currentTimeMillis());
Review comment:
As we discussed, please remove the debug logs if not absolutely necessary for long term usage.
##########
File path: helix-core/src/main/java/org/apache/helix/tools/ClusterVerifiers/ZkHelixClusterVerifier.java
##########
@@ -187,6 +190,14 @@ public boolean verifyByZkCallback() {
* @return
*/
public boolean verifyByPolling(long timeout, long period) {
Review comment:
I believe you can and should add the same wait for verifyByZkCallback too.
The logic of verifyByZkCallback is that
1. subscribe to changes.
2. no matter if change happens, verify the state for once.
3. if subscription results in some notification, then verify in parallel.
4. any of the paths succeed, then the verify succeeds.
In this case, we shall wait until the first subscribe to ensure the waitTillVerify config takes effect. This won't cause any problem with verifying logic. Since if the change happens before we subscribe, we are still fine because the first default verify check will be done once at least.
----------------------------------------------------------------
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
[GitHub] [helix] pkuwm merged pull request #1328: Enhance ZkHelixVerifier and its related subclass to take a default wait period
Posted by GitBox <gi...@apache.org>.
pkuwm merged pull request #1328:
URL: https://github.com/apache/helix/pull/1328
----------------------------------------------------------------
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
[GitHub] [helix] jiajunwang edited a comment on pull request #1328: Implement TestBestPossibleExternalViewVerifier
Posted by GitBox <gi...@apache.org>.
jiajunwang edited a comment on pull request #1328:
URL: https://github.com/apache/helix/pull/1328#issuecomment-691374998
> This diff is approve, please help to merge into trunk.
>
> > TestBestPossibleExternalViewVerifier would have a default COOL_DOWN period
> > before verifyByPolling. Due to #526, currently we have randow sleep in the
> > test before verifyByPolling. We intend to use TestBestPossibeVerifier
> > instead in our unit test. While keep using BestPossibleExternalViewVerifier
> > in production code.
@kaisun2000 please update the comment, it has diverged from the code change quite a lot. Please do take care of the final comment message, once it is in, we don't have a way to modify except reverting the PR.
There are good reasons that we request the author to confirm the final message, instead of just copy-paste from the PR description. And the author should not copy-paste either.
----------------------------------------------------------------
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
[GitHub] [helix] jiajunwang edited a comment on pull request #1328: Implement TestBestPossibleExternalViewVerifier
Posted by GitBox <gi...@apache.org>.
jiajunwang edited a comment on pull request #1328:
URL: https://github.com/apache/helix/pull/1328#issuecomment-691374998
----------------------------------------------------------------
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
[GitHub] [helix] kaisun2000 commented on a change in pull request #1328: Implement TestBestPossibleExternalViewVerifier
Posted by GitBox <gi...@apache.org>.
kaisun2000 commented on a change in pull request #1328:
URL: https://github.com/apache/helix/pull/1328#discussion_r485985179
##########
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:
removed.
----------------------------------------------------------------
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
[GitHub] [helix] kaisun2000 commented on a change in pull request #1328: Implement TestBestPossibleExternalViewVerifier
Posted by GitBox <gi...@apache.org>.
kaisun2000 commented on a change in pull request #1328:
URL: https://github.com/apache/helix/pull/1328#discussion_r485985231
##########
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:
done.
----------------------------------------------------------------
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
[GitHub] [helix] kaisun2000 commented on a change in pull request #1328: Implement TestBestPossibleExternalViewVerifier
Posted by GitBox <gi...@apache.org>.
kaisun2000 commented on a change in pull request #1328:
URL: https://github.com/apache/helix/pull/1328#discussion_r480350436
##########
File path: helix-core/src/main/java/org/apache/helix/tools/ClusterVerifiers/BestPossibleExternalViewVerifier.java
##########
@@ -224,12 +224,15 @@ public boolean verifyByZkCallback(long timeout) {
@Override
protected synchronized boolean verifyState() {
try {
+ LOG.debug("Verifier start verifyState at {}", System.currentTimeMillis());
Review comment:
good point. Removed time stamp for all of them.
----------------------------------------------------------------
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
[GitHub] [helix] jiajunwang edited a comment on pull request #1328: Implement TestBestPossibleExternalViewVerifier
Posted by GitBox <gi...@apache.org>.
jiajunwang edited a comment on pull request #1328:
URL: https://github.com/apache/helix/pull/1328#issuecomment-691374998
> This diff is approve, please help to merge into trunk.
>
> > TestBestPossibleExternalViewVerifier would have a default COOL_DOWN period
> > before verifyByPolling. Due to #526, currently we have randow sleep in the
> > test before verifyByPolling. We intend to use TestBestPossibeVerifier
> > instead in our unit test. While keep using BestPossibleExternalViewVerifier
> > in production code.
@kaisun2000 please update the comment, it has diverged from the code change quite a lot. Please do take care of the final comment message, once it is in, we don't have a way to modify except reverting the PR.
There are good reasons that we request the author to confirm the final message, instead of just copy-paste from the PR description. And the author should not copy-paste either.
----------------------------------------------------------------
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
[GitHub] [helix] kaisun2000 commented on a change in pull request #1328: Implement TestBestPossibleExternalViewVerifier
Posted by GitBox <gi...@apache.org>.
kaisun2000 commented on a change in pull request #1328:
URL: https://github.com/apache/helix/pull/1328#discussion_r485985090
##########
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:
fixed.
----------------------------------------------------------------
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