You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@helix.apache.org by "limbooverlambda (via GitHub)" <gi...@apache.org> on 2023/04/26 00:56:46 UTC
[GitHub] [helix] limbooverlambda opened a new pull request, #2458: Refactor testHelixViewAggregator
limbooverlambda opened a new pull request, #2458:
URL: https://github.com/apache/helix/pull/2458
### Issues
Refactoring the testHelixViewAggregator by removing Thread.sleep and using awaitility within the TestHelper to specify a poll frequency for verifications.
- [x] My PR addresses the following Helix issues and references them in the PR description:
#2438
### Description
While looking at the failed test run logs it was observed that one of the rebalance operations (the third rebalance call) was not getting invoked. The assertions to verify the MockViewClusterSpectator state failed after the `Thread.sleep() `expired. The likely hypothesis is that the timeout duration of 7 seconds was not sufficient for the ClusterSpectator state to reach steady state. So instead of leveraging `Thread.sleep() `to wait for the cluster to reach steady state in this PR we are using `TestHelper.verify` to check for the desired state.
### Tests
- The following is the result of the "mvn test" command on the appropriate module:
[INFO] -------------------------------------------------------
[INFO] T E S T S
[INFO] -------------------------------------------------------
[INFO] Running TestSuite
Start zookeeper at localhost:2183 in thread main
Creating resources ...
Rebalancing resources ...
Rebalancing resources ...
Rebalancing resources ...
AfterClass: TestHelixViewAggregator called.
AfterClass: TestSourceClusterDataProvider called.
AfterClass: TestViewClusterRefresher called.
AfterClass: TestViewClusterDataCache called.
Shut down zookeeper at port 2183 in thread main
[INFO] Tests run: 15, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 73.581 s - in TestSuite
[INFO]
[INFO] Results:
[INFO]
[INFO] Tests run: 15, Failures: 0, Errors: 0, Skipped: 0
[INFO]
### 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.
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
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] xyuanlu commented on a diff in pull request #2458: Refactor testHelixViewAggregator
Posted by "xyuanlu (via GitHub)" <gi...@apache.org>.
xyuanlu commented on code in PR #2458:
URL: https://github.com/apache/helix/pull/2458#discussion_r1178215719
##########
helix-core/src/test/java/org/apache/helix/TestHelper.java:
##########
@@ -803,19 +803,15 @@ public interface Verifier {
}
public static boolean verify(Verifier verifier, long timeout) throws Exception {
- long start = System.currentTimeMillis();
- do {
- boolean result = verifier.verify();
- boolean isTimedout = (System.currentTimeMillis() - start) > timeout;
- if (result || isTimedout) {
- if (isTimedout && !result) {
- LOG.error("verifier time out, consider try longer timeout, stack trace{}",
- Arrays.asList(Thread.currentThread().getStackTrace()));
- }
- return result;
- }
- Thread.sleep(50);
- } while (true);
+ try {
+ with().pollInterval(50, TimeUnit.MILLISECONDS).atMost(timeout, TimeUnit.MILLISECONDS).await()
+ .until(verifier::verify);
+ return verifier.verify();
+ } catch (Exception ex) {
+ LOG.error("verifier time out, consider try longer timeout, stack trace{}",
Review Comment:
Thanks for explain. I feel like there is no need to log here. Exception will print stack trace.
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
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] xyuanlu commented on pull request #2458: Refactor testHelixViewAggregator
Posted by "xyuanlu (via GitHub)" <gi...@apache.org>.
xyuanlu commented on PR #2458:
URL: https://github.com/apache/helix/pull/2458#issuecomment-1530998475
Thanks for making the change :D
As discussed offline, could you please run this PR ~20 times in a separate fork and check test rate?
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
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] xyuanlu commented on a diff in pull request #2458: Refactor testHelixViewAggregator
Posted by "xyuanlu (via GitHub)" <gi...@apache.org>.
xyuanlu commented on code in PR #2458:
URL: https://github.com/apache/helix/pull/2458#discussion_r1177318694
##########
helix-core/src/test/java/org/apache/helix/TestHelper.java:
##########
@@ -803,19 +803,15 @@ public interface Verifier {
}
public static boolean verify(Verifier verifier, long timeout) throws Exception {
- long start = System.currentTimeMillis();
- do {
- boolean result = verifier.verify();
- boolean isTimedout = (System.currentTimeMillis() - start) > timeout;
- if (result || isTimedout) {
- if (isTimedout && !result) {
- LOG.error("verifier time out, consider try longer timeout, stack trace{}",
- Arrays.asList(Thread.currentThread().getStackTrace()));
- }
- return result;
- }
- Thread.sleep(50);
- } while (true);
+ try {
+ with().pollInterval(50, TimeUnit.MILLISECONDS).atMost(timeout, TimeUnit.MILLISECONDS).await()
+ .until(verifier::verify);
+ return verifier.verify();
+ } catch (Exception ex) {
+ LOG.error("verifier time out, consider try longer timeout, stack trace{}",
Review Comment:
I think when verify times out, it will throw exception and log stack trace. Catch - log and re throw is not needed.
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
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] limbooverlambda commented on a diff in pull request #2458: Refactor testHelixViewAggregator
Posted by "limbooverlambda (via GitHub)" <gi...@apache.org>.
limbooverlambda commented on code in PR #2458:
URL: https://github.com/apache/helix/pull/2458#discussion_r1178229716
##########
helix-core/src/test/java/org/apache/helix/TestHelper.java:
##########
@@ -803,19 +803,15 @@ public interface Verifier {
}
public static boolean verify(Verifier verifier, long timeout) throws Exception {
- long start = System.currentTimeMillis();
- do {
- boolean result = verifier.verify();
- boolean isTimedout = (System.currentTimeMillis() - start) > timeout;
- if (result || isTimedout) {
- if (isTimedout && !result) {
- LOG.error("verifier time out, consider try longer timeout, stack trace{}",
- Arrays.asList(Thread.currentThread().getStackTrace()));
- }
- return result;
- }
- Thread.sleep(50);
- } while (true);
+ try {
+ with().pollInterval(50, TimeUnit.MILLISECONDS).atMost(timeout, TimeUnit.MILLISECONDS).await()
+ .until(verifier::verify);
+ return verifier.verify();
+ } catch (Exception ex) {
+ LOG.error("verifier time out, consider try longer timeout, stack trace{}",
Review Comment:
Great. Removing the catch-rethrow
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
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] limbooverlambda closed pull request #2458: Refactor testHelixViewAggregator
Posted by "limbooverlambda (via GitHub)" <gi...@apache.org>.
limbooverlambda closed pull request #2458: Refactor testHelixViewAggregator
URL: https://github.com/apache/helix/pull/2458
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
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] xyuanlu commented on pull request #2458: Refactor testHelixViewAggregator
Posted by "xyuanlu (via GitHub)" <gi...@apache.org>.
xyuanlu commented on PR #2458:
URL: https://github.com/apache/helix/pull/2458#issuecomment-1546440135
As discussed offline, let's have CI run in another branch in non-apache PR CI.
Could you please squeeze these commits? THX :D
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
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] limbooverlambda commented on a diff in pull request #2458: Refactor testHelixViewAggregator
Posted by "limbooverlambda (via GitHub)" <gi...@apache.org>.
limbooverlambda commented on code in PR #2458:
URL: https://github.com/apache/helix/pull/2458#discussion_r1178214977
##########
helix-core/src/test/java/org/apache/helix/TestHelper.java:
##########
@@ -803,19 +803,15 @@ public interface Verifier {
}
public static boolean verify(Verifier verifier, long timeout) throws Exception {
- long start = System.currentTimeMillis();
- do {
- boolean result = verifier.verify();
- boolean isTimedout = (System.currentTimeMillis() - start) > timeout;
- if (result || isTimedout) {
- if (isTimedout && !result) {
- LOG.error("verifier time out, consider try longer timeout, stack trace{}",
- Arrays.asList(Thread.currentThread().getStackTrace()));
- }
- return result;
- }
- Thread.sleep(50);
- } while (true);
+ try {
+ with().pollInterval(50, TimeUnit.MILLISECONDS).atMost(timeout, TimeUnit.MILLISECONDS).await()
+ .until(verifier::verify);
+ return verifier.verify();
+ } catch (Exception ex) {
+ LOG.error("verifier time out, consider try longer timeout, stack trace{}",
Review Comment:
In the previous implementation it looks like upon timeout, a false is returned upon the expiration of the timeout (preceded by a log err indicating that the verification has timed out). In this iteration, we are throwing the exception back but the catch-rethrow is just to render the log line.
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
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] xyuanlu commented on a diff in pull request #2458: Refactor testHelixViewAggregator
Posted by "xyuanlu (via GitHub)" <gi...@apache.org>.
xyuanlu commented on code in PR #2458:
URL: https://github.com/apache/helix/pull/2458#discussion_r1178686051
##########
helix-core/src/test/java/org/apache/helix/TestHelper.java:
##########
@@ -803,19 +803,9 @@ public interface Verifier {
}
public static boolean verify(Verifier verifier, long timeout) throws Exception {
- long start = System.currentTimeMillis();
- do {
- boolean result = verifier.verify();
- boolean isTimedout = (System.currentTimeMillis() - start) > timeout;
- if (result || isTimedout) {
- if (isTimedout && !result) {
- LOG.error("verifier time out, consider try longer timeout, stack trace{}",
- Arrays.asList(Thread.currentThread().getStackTrace()));
- }
- return result;
- }
- Thread.sleep(50);
- } while (true);
+ with().pollInterval(50, TimeUnit.MILLISECONDS).atMost(timeout, TimeUnit.MILLISECONDS).await()
Review Comment:
Please correct me if I am wrong. I think Verifier provided same functionality as await?
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
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] rahulrane50 commented on a diff in pull request #2458: Refactor testHelixViewAggregator
Posted by "rahulrane50 (via GitHub)" <gi...@apache.org>.
rahulrane50 commented on code in PR #2458:
URL: https://github.com/apache/helix/pull/2458#discussion_r1189272252
##########
helix-view-aggregator/src/test/java/org/apache/helix/view/integration/TestHelixViewAggregator.java:
##########
@@ -247,11 +274,16 @@ private void resetViewClusterConfig(int refreshPeriod, List<PropertyType> proper
_configAccessor.setClusterConfig(viewClusterName, viewClusterConfig);
}
- private void verifyViewClusterEventChanges(boolean externalViewChange,
- boolean instanceConfigChange, boolean liveInstancesChange) {
- Assert.assertEquals(_monitor.getExternalViewChangeCount() > 0, externalViewChange);
- Assert.assertEquals(_monitor.getInstanceConfigChangeCount() > 0, instanceConfigChange);
- Assert.assertEquals(_monitor.getLiveInstanceChangeCount() > 0, liveInstancesChange);
+ private Predicate<MockViewClusterSpectator> hasExternalViewChanges() {
Review Comment:
nit : nice use of predicate! may be we can add this method in MockViewClusterSpectator itself!
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
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] limbooverlambda commented on pull request #2458: Refactor testHelixViewAggregator
Posted by "limbooverlambda (via GitHub)" <gi...@apache.org>.
limbooverlambda commented on PR #2458:
URL: https://github.com/apache/helix/pull/2458#issuecomment-1553402507
The usage of Awaitility in core was causing test failures across multiple modules. Closing this PR, will be creating another one with the appropriate changes.
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
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