You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by GitBox <gi...@apache.org> on 2021/01/21 09:27:22 UTC
[GitHub] [pulsar] eolivelli opened a new pull request #9258: Fix flaky DiscoveryServiceTest
eolivelli opened a new pull request #9258:
URL: https://github.com/apache/pulsar/pull/9258
Fix this flaky test DiscoveryServiceTest
The test may result flaky on slow machines because the update of the cache happens asynchronously.
Change:
- wait for the cache to be updated before performing the assertions
- add more information during the failure
- add awaitility test dependency to the pulsar-discovery-service module
This is the error, quite frequent on GitHub Actions
```
java.lang.AssertionError: did not expect [pulsar://broker-:150000] but found [pulsar://broker-:150000]
at org.testng.Assert.fail(Assert.java:99)
at org.testng.Assert.failEquals(Assert.java:1041)
at org.testng.Assert.assertNotEqualsImpl(Assert.java:147)
at org.testng.Assert.assertNotEquals(Assert.java:1531)
at org.testng.Assert.assertNotEquals(Assert.java:1535)
at org.apache.pulsar.discovery.service.DiscoveryServiceTest.testBrokerDiscoveryRoundRobin(DiscoveryServiceTest.java:95)
at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
at java.lang.reflect.Method.invoke(Method.java:498)
at org.testng.internal.MethodInvocationHelper.invokeMethod(MethodInvocationHelper.java:132)
at org.testng.internal.InvokeMethodRunnable.runOne(InvokeMethodRunnable.java:45)
at org.testng.internal.InvokeMethodRunnable.call(InvokeMethodRunnable.java:73)
at org.testng.internal.InvokeMethodRunnable.call(InvokeMethodRunnable.java:11)
at java.util.concurrent.FutureTask.run(FutureTask.java:266)
at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149)
at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624)
at java.lang.Thread.run(Thread.java:748)
```
----------------------------------------------------------------
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
[GitHub] [pulsar] eolivelli commented on a change in pull request #9258: Fix flaky DiscoveryServiceTest
Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #9258:
URL: https://github.com/apache/pulsar/pull/9258#discussion_r561730217
##########
File path: pulsar-discovery-service/src/test/java/org/apache/pulsar/discovery/service/DiscoveryServiceTest.java
##########
@@ -90,9 +91,11 @@ private void clean() throws Exception {
public void testBrokerDiscoveryRoundRobin() throws Exception {
addBrokerToZk(5);
String prevUrl = null;
+ BrokerDiscoveryProvider discoveryProvider = service.getDiscoveryProvider();
+ assertEquals(discoveryProvider.getAvailableBrokers().size(), 5);
Review comment:
the wait is done inside addBrokerToZk (that is the actual fix)
I added this line in order to double check in case of a new failure,
I can remove it, is is basically redundant
----------------------------------------------------------------
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
[GitHub] [pulsar] lhotari commented on a change in pull request #9258: Fix flaky DiscoveryServiceTest
Posted by GitBox <gi...@apache.org>.
lhotari commented on a change in pull request #9258:
URL: https://github.com/apache/pulsar/pull/9258#discussion_r561725600
##########
File path: pulsar-discovery-service/src/test/java/org/apache/pulsar/discovery/service/DiscoveryServiceTest.java
##########
@@ -90,9 +91,11 @@ private void clean() throws Exception {
public void testBrokerDiscoveryRoundRobin() throws Exception {
addBrokerToZk(5);
String prevUrl = null;
+ BrokerDiscoveryProvider discoveryProvider = service.getDiscoveryProvider();
+ assertEquals(discoveryProvider.getAvailableBrokers().size(), 5);
Review comment:
would there be a need to use Awaitility to make sure that there are 5 available brokers?
##########
File path: pulsar-discovery-service/src/test/java/org/apache/pulsar/discovery/service/DiscoveryServiceTest.java
##########
@@ -245,6 +248,9 @@ private void addBrokerToZk(int number) throws Exception {
ZooKeeperChildrenCache availableBrokersCache = (ZooKeeperChildrenCache) field
.get(service.getDiscoveryProvider().localZkCache);
availableBrokersCache.reloadCache(LOADBALANCE_BROKERS_ROOT);
+
+ Awaitility.await().atMost(10, TimeUnit.SECONDS).until(()
Review comment:
I've been wondering if we should get rid of `.atMost(10, TimeUnit.SECONDS)` parts and simply rely on the [the Awaitility defaults](https://github.com/awaitility/awaitility/wiki/Usage#defaults). The default value for atMost is 10 seconds.
----------------------------------------------------------------
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
[GitHub] [pulsar] eolivelli commented on pull request #9258: Fix flaky DiscoveryServiceTest
Posted by GitBox <gi...@apache.org>.
eolivelli commented on pull request #9258:
URL: https://github.com/apache/pulsar/pull/9258#issuecomment-768105185
/pulsarbot run-failure-checks
----------------------------------------------------------------
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
[GitHub] [pulsar] eolivelli commented on a change in pull request #9258: Fix flaky DiscoveryServiceTest
Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #9258:
URL: https://github.com/apache/pulsar/pull/9258#discussion_r561730994
##########
File path: pulsar-discovery-service/src/test/java/org/apache/pulsar/discovery/service/DiscoveryServiceTest.java
##########
@@ -245,6 +248,9 @@ private void addBrokerToZk(int number) throws Exception {
ZooKeeperChildrenCache availableBrokersCache = (ZooKeeperChildrenCache) field
.get(service.getDiscoveryProvider().localZkCache);
availableBrokersCache.reloadCache(LOADBALANCE_BROKERS_ROOT);
+
+ Awaitility.await().atMost(10, TimeUnit.SECONDS).until(()
Review comment:
okay, makes sense
----------------------------------------------------------------
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
[GitHub] [pulsar] eolivelli commented on pull request #9258: Fix flaky DiscoveryServiceTest
Posted by GitBox <gi...@apache.org>.
eolivelli commented on pull request #9258:
URL: https://github.com/apache/pulsar/pull/9258#issuecomment-765529238
/pulsarbot run-failure-checks
----------------------------------------------------------------
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
[GitHub] [pulsar] lhotari commented on a change in pull request #9258: Fix flaky DiscoveryServiceTest
Posted by GitBox <gi...@apache.org>.
lhotari commented on a change in pull request #9258:
URL: https://github.com/apache/pulsar/pull/9258#discussion_r561725600
##########
File path: pulsar-discovery-service/src/test/java/org/apache/pulsar/discovery/service/DiscoveryServiceTest.java
##########
@@ -90,9 +91,11 @@ private void clean() throws Exception {
public void testBrokerDiscoveryRoundRobin() throws Exception {
addBrokerToZk(5);
String prevUrl = null;
+ BrokerDiscoveryProvider discoveryProvider = service.getDiscoveryProvider();
+ assertEquals(discoveryProvider.getAvailableBrokers().size(), 5);
Review comment:
would there be a need to use Awaitility to make sure that there are 5 available brokers?
----------------------------------------------------------------
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
[GitHub] [pulsar] eolivelli commented on pull request #9258: Fix flaky DiscoveryServiceTest
Posted by GitBox <gi...@apache.org>.
eolivelli commented on pull request #9258:
URL: https://github.com/apache/pulsar/pull/9258#issuecomment-765248702
/pulsarbot run-failure-checks
----------------------------------------------------------------
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
[GitHub] [pulsar] sijie merged pull request #9258: Fix flaky DiscoveryServiceTest
Posted by GitBox <gi...@apache.org>.
sijie merged pull request #9258:
URL: https://github.com/apache/pulsar/pull/9258
----------------------------------------------------------------
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
[GitHub] [pulsar] eolivelli commented on pull request #9258: Fix flaky DiscoveryServiceTest
Posted by GitBox <gi...@apache.org>.
eolivelli commented on pull request #9258:
URL: https://github.com/apache/pulsar/pull/9258#issuecomment-764645656
/pulsarbot run-failure-checks
----------------------------------------------------------------
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
[GitHub] [pulsar] lhotari commented on a change in pull request #9258: Fix flaky DiscoveryServiceTest
Posted by GitBox <gi...@apache.org>.
lhotari commented on a change in pull request #9258:
URL: https://github.com/apache/pulsar/pull/9258#discussion_r561727574
##########
File path: pulsar-discovery-service/src/test/java/org/apache/pulsar/discovery/service/DiscoveryServiceTest.java
##########
@@ -245,6 +248,9 @@ private void addBrokerToZk(int number) throws Exception {
ZooKeeperChildrenCache availableBrokersCache = (ZooKeeperChildrenCache) field
.get(service.getDiscoveryProvider().localZkCache);
availableBrokersCache.reloadCache(LOADBALANCE_BROKERS_ROOT);
+
+ Awaitility.await().atMost(10, TimeUnit.SECONDS).until(()
Review comment:
I've been wondering if we should get rid of `.atMost(10, TimeUnit.SECONDS)` parts and simply rely on the [the Awaitility defaults](https://github.com/awaitility/awaitility/wiki/Usage#defaults). The default value for atMost is 10 seconds.
----------------------------------------------------------------
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
[GitHub] [pulsar] eolivelli commented on a change in pull request #9258: Fix flaky DiscoveryServiceTest
Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #9258:
URL: https://github.com/apache/pulsar/pull/9258#discussion_r561730217
##########
File path: pulsar-discovery-service/src/test/java/org/apache/pulsar/discovery/service/DiscoveryServiceTest.java
##########
@@ -90,9 +91,11 @@ private void clean() throws Exception {
public void testBrokerDiscoveryRoundRobin() throws Exception {
addBrokerToZk(5);
String prevUrl = null;
+ BrokerDiscoveryProvider discoveryProvider = service.getDiscoveryProvider();
+ assertEquals(discoveryProvider.getAvailableBrokers().size(), 5);
Review comment:
the wait is done inside addBrokerToZk (that is the actual fix)
I added this line in order to double check in case of a new failure,
I can remove it, is is basically redundant
##########
File path: pulsar-discovery-service/src/test/java/org/apache/pulsar/discovery/service/DiscoveryServiceTest.java
##########
@@ -245,6 +248,9 @@ private void addBrokerToZk(int number) throws Exception {
ZooKeeperChildrenCache availableBrokersCache = (ZooKeeperChildrenCache) field
.get(service.getDiscoveryProvider().localZkCache);
availableBrokersCache.reloadCache(LOADBALANCE_BROKERS_ROOT);
+
+ Awaitility.await().atMost(10, TimeUnit.SECONDS).until(()
Review comment:
okay, makes sense
----------------------------------------------------------------
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
[GitHub] [pulsar] eolivelli commented on pull request #9258: Fix flaky DiscoveryServiceTest
Posted by GitBox <gi...@apache.org>.
eolivelli commented on pull request #9258:
URL: https://github.com/apache/pulsar/pull/9258#issuecomment-764645656
----------------------------------------------------------------
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
[GitHub] [pulsar] eolivelli commented on pull request #9258: Fix flaky DiscoveryServiceTest
Posted by GitBox <gi...@apache.org>.
eolivelli commented on pull request #9258:
URL: https://github.com/apache/pulsar/pull/9258#issuecomment-765202248
/pulsarbot run-failure-checks
----------------------------------------------------------------
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
[GitHub] [pulsar] eolivelli commented on pull request #9258: Fix flaky DiscoveryServiceTest
Posted by GitBox <gi...@apache.org>.
eolivelli commented on pull request #9258:
URL: https://github.com/apache/pulsar/pull/9258#issuecomment-764727745
/pulsarbot run-failure-checks
----------------------------------------------------------------
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