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/10/29 00:44:19 UTC
[GitHub] [helix] NealSun96 commented on a change in pull request #1497: Fix TestRoutingTableProviderPeriodicRefresh.
NealSun96 commented on a change in pull request #1497:
URL: https://github.com/apache/helix/pull/1497#discussion_r513843732
##########
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);
Review comment:
Could you elaborate on the reason of failure before? Is it because periodic refresh sometimes take a bit more than 1000, so it's possible that `newRefreshCount == prevRefreshCount`; or, is it because periodic refresh may take less time, so it's possible that `newRefreshCount == prevRefreshCount + 2`?
##########
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:
While this eliminates flakiness, would it be too loose to ensure correctness?
----------------------------------------------------------------
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