NealSun96 commented on a change in pull request #1497:
URL: https://github.com/apache/helix/pull/1497#discussion_r514429911
##########
File path:
helix-core/src/test/java/org/apache/helix/integration/spectator/TestRoutingTableProviderPeriodicRefresh.java
##########
@@ -203,34 +208,32 @@ synchronized int getRefreshCount() {
@Test
public void testPeriodicRefresh() throws InterruptedException {
- // Wait so that initial refreshes finish (not triggered by periodic
refresh timer)
- Thread.sleep(1000L);
+ // Wait to ensure that the initial refreshes finish (not triggered by
periodic refresh timer)
+ Thread.sleep(REFRESH_PERIOD_MS * 2);
// Test short refresh
int prevRefreshCount = _routingTableProvider.getRefreshCount();
- // Wait for one timer duration
- Thread.sleep(1000L);
- // The timer should have gone off, incrementing the refresh count
- Assert.assertEquals(_routingTableProvider.getRefreshCount(),
prevRefreshCount + 1);
+ // Wait for more than one timer duration
+ Thread.sleep((long) (REFRESH_PERIOD_MS * 1.5));
+ // The timer should have gone off, incrementing the refresh count by one
or two depends on the
+ // timing.
+ int newRefreshCount = _routingTableProvider.getRefreshCount();
+ Assert.assertTrue(
+ newRefreshCount == prevRefreshCount + 1 || newRefreshCount ==
prevRefreshCount + 2);
Review comment:
This comment is tied with my other comment. If we already know it's
flaky between 1 or 2 refreshes, why don't we just `assert(+1 || +2)`, while
keeping `sleep(1000)`?
If we change it to `sleep(1500)`, we are opening doors to "slow refresh"
cases. If a refresh doesn't respect the refresh period, and instead took around
1500, the previous version of the test will catch it, but now it won't. Like
you said, maybe even 1500 is fine since there is no hard requirement, we are
just ensuring the refresh happens, but it feels to me that correctness check is
relaxed.
----------------------------------------------------------------
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]
