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:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
