Replace deprecated verifier with new set of Verifiers#2505
Conversation
| result = verifier.verifyByZkCallback(); | ||
| result = TestHelper.verify(() -> { | ||
| return verifier.verifyByZkCallback(); | ||
| }, 100); |
There was a problem hiding this comment.
Actually 100, won't help much, the verify method does the following:
- Record startTime = currentTime
- Invoke verifier.verifyByZkCallback()
2.1 Wait for max 30 seconds(default timeout) for the state to be verified - If currentTime - startTime > 100 repeat 1.
So I think we need to have time out > (30 seconds + some buffer) if we want the verification to be retried. If we don't want the verification to be retried(which is the current implementation), then we need to use verifier.verifyByZkCallback(30seconds default timeout + 100 milliseconds) instead of verifier.verifyByZkCallback().
Apologies for the previous comment as I didn't read through the TestHelper.verify method code.
There was a problem hiding this comment.
I think, i don't need it re=tried as earlier code just did sleep and moved on.
This is also the reason, I like to stick to the scope of the change to be what I set out with.
There was a problem hiding this comment.
Still re-try is not required, can you please review it one final time.
There was a problem hiding this comment.
Ok. Lets stick to original one or use callback with timeout that has 30 seconds + 100 milliseconds set.
Replace the deprecated verifier with new set of verifiers. --------- Co-authored-by: Komal Desai <kdesai@kdesai-mn1.linkedin.biz>
Replace the deprecated verifier with new set of verifiers. --------- Co-authored-by: Komal Desai <kdesai@kdesai-mn1.linkedin.biz>
Replace the deprecated verifier with new set of verifiers. --------- Co-authored-by: Komal Desai <kdesai@kdesai-mn1.linkedin.biz>
Issues
Fixes TestDistributedControllerManager is using Deprecated ClusterStateVerifier #2485
Description
While debugging the failure for the test case in DistributedController test case, realized we are using deprecated verifier. So let us first use the right verifier and see if it helps with resolving the temporary failures. These are all integration tests which depends on ZK timing and so not sure if just replacing correct verifier will help. But first order problem is to use right code.
Tests
(List the names of added unit/integration tests)
I created a branch and ran the test 4 times to make sure that there is no regression with using different verifier.
Here is the link to the branch with testing:
Fix TestDistributedController to use the proper verifier. Fixes #2485 desaikomal/helix#2
Changes that Break Backward Compatibility (Optional)
(Consider including all behavior changes for public methods or API. Also include these changes in merge description so that other developers are aware of these changes. This allows them to make relevant code changes in feature branches accounting for the new method/API behavior.)
Documentation (Optional)
(Link the GitHub wiki you added)
Commits
Code Quality
(helix-style-intellij.xml if IntelliJ IDE is used)