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/05/15 13:05:07 UTC
[GitHub] [pulsar] lhotari opened a new pull request #10599: [Tests] Fix flaky test GracefulExecutorServicesShutdownTest
lhotari opened a new pull request #10599:
URL: https://github.com/apache/pulsar/pull/10599
### Motivation
`GracefulExecutorServicesShutdownTest`'s `shouldTerminateWhenFutureIsCancelled` remains flaky after #10592 .
Example failure: https://github.com/apache/pulsar/pull/10598/checks?check_run_id=2590214027#step:10:1341
### Modifications
- fix race condition in test by adding a CountDownLatch to verify that execution has entered the awaitTermination method before the future is cancelled.
- use CompletableFuture for awaitTerminationInterrupted check.
--
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 pull request #10599: [Tests] Fix flaky test GracefulExecutorServicesShutdownTest
Posted by GitBox <gi...@apache.org>.
lhotari commented on pull request #10599:
URL: https://github.com/apache/pulsar/pull/10599#issuecomment-841793435
> But after call countdown, main thread can cancel the future, that means the Interruption exception can probably be caught by outer catch.
> You can test it locally by add some time-consuming action after countdown.
I don't see how this could happen. Interrupting a thread doesn't stop execution. It sets the interrupted status and interrupts any blocking methods such as sleep or blocking IO with an InterruptedException. Calling countDown will never be interrupted.
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] linlinnn commented on a change in pull request #10599: [Tests] Fix flaky test GracefulExecutorServicesShutdownTest
Posted by GitBox <gi...@apache.org>.
linlinnn commented on a change in pull request #10599:
URL: https://github.com/apache/pulsar/pull/10599#discussion_r632950133
##########
File path: pulsar-broker/src/test/java/org/apache/pulsar/broker/service/GracefulExecutorServicesShutdownTest.java
##########
@@ -120,25 +121,28 @@ public void shouldWaitForExecutorToTerminate() throws ExecutionException, Interr
@Test
- public void shouldTerminateWhenFutureIsCancelled() throws InterruptedException {
+ public void shouldTerminateWhenFutureIsCancelled() throws InterruptedException, ExecutionException {
// given
GracefulExecutorServicesShutdown shutdown = GracefulExecutorServicesShutdown.initiate();
shutdown.timeout(Duration.ofMillis(15000));
ExecutorService executorService = mock(ExecutorService.class);
when(executorService.isShutdown()).thenReturn(true);
AtomicBoolean terminated = new AtomicBoolean();
- AtomicBoolean awaitTerminationInterrupted = new AtomicBoolean();
+ CompletableFuture<Boolean> awaitTerminationInterrupted = new CompletableFuture<>();
when(executorService.isTerminated()).thenAnswer(invocation -> terminated.get());
+ CountDownLatch awaitingTerminationEntered = new CountDownLatch(1);
when(executorService.awaitTermination(anyLong(), any())).thenAnswer(invocation -> {
long timeout = invocation.getArgument(0);
TimeUnit unit = invocation.getArgument(1);
+ awaitingTerminationEntered.countDown();
Review comment:
nice, I happen to be about to solve this problem
--
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] linlinnn commented on a change in pull request #10599: [Tests] Fix flaky test GracefulExecutorServicesShutdownTest
Posted by GitBox <gi...@apache.org>.
linlinnn commented on a change in pull request #10599:
URL: https://github.com/apache/pulsar/pull/10599#discussion_r632976110
##########
File path: pulsar-broker/src/test/java/org/apache/pulsar/broker/service/GracefulExecutorServicesShutdownTest.java
##########
@@ -120,25 +121,28 @@ public void shouldWaitForExecutorToTerminate() throws ExecutionException, Interr
@Test
- public void shouldTerminateWhenFutureIsCancelled() throws InterruptedException {
+ public void shouldTerminateWhenFutureIsCancelled() throws InterruptedException, ExecutionException {
// given
GracefulExecutorServicesShutdown shutdown = GracefulExecutorServicesShutdown.initiate();
shutdown.timeout(Duration.ofMillis(15000));
ExecutorService executorService = mock(ExecutorService.class);
when(executorService.isShutdown()).thenReturn(true);
AtomicBoolean terminated = new AtomicBoolean();
- AtomicBoolean awaitTerminationInterrupted = new AtomicBoolean();
+ CompletableFuture<Boolean> awaitTerminationInterrupted = new CompletableFuture<>();
when(executorService.isTerminated()).thenAnswer(invocation -> terminated.get());
+ CountDownLatch awaitingTerminationEntered = new CountDownLatch(1);
when(executorService.awaitTermination(anyLong(), any())).thenAnswer(invocation -> {
long timeout = invocation.getArgument(0);
TimeUnit unit = invocation.getArgument(1);
+ awaitingTerminationEntered.countDown();
Review comment:
1. awaitingTerminationEntered.countDown() and if the thread do not run into try block.
2. future.cancel(false);
then awaitTerminationInterrupted maybe never complete.
--
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] linlinnn commented on pull request #10599: [Tests] Fix flaky test GracefulExecutorServicesShutdownTest
Posted by GitBox <gi...@apache.org>.
linlinnn commented on pull request #10599:
URL: https://github.com/apache/pulsar/pull/10599#issuecomment-841785603
> What caused the flakiness was that this condition caused the execution to never get to the Thread.sleep when the thread was already interrupted:
Yes, I agree that.
But after call countdown, main thread can cancel the future, that means the Interruption exception can probably be caught by outer catch.
You can test it locally by add some time-consuming action after countdown.
--
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] linlinnn commented on pull request #10599: [Tests] Fix flaky test GracefulExecutorServicesShutdownTest
Posted by GitBox <gi...@apache.org>.
linlinnn commented on pull request #10599:
URL: https://github.com/apache/pulsar/pull/10599#issuecomment-841789530
ok
--
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] linlinnn commented on a change in pull request #10599: [Tests] Fix flaky test GracefulExecutorServicesShutdownTest
Posted by GitBox <gi...@apache.org>.
linlinnn commented on a change in pull request #10599:
URL: https://github.com/apache/pulsar/pull/10599#discussion_r632949407
##########
File path: pulsar-broker/src/test/java/org/apache/pulsar/broker/service/GracefulExecutorServicesShutdownTest.java
##########
@@ -120,25 +121,28 @@ public void shouldWaitForExecutorToTerminate() throws ExecutionException, Interr
@Test
- public void shouldTerminateWhenFutureIsCancelled() throws InterruptedException {
+ public void shouldTerminateWhenFutureIsCancelled() throws InterruptedException, ExecutionException {
// given
GracefulExecutorServicesShutdown shutdown = GracefulExecutorServicesShutdown.initiate();
shutdown.timeout(Duration.ofMillis(15000));
ExecutorService executorService = mock(ExecutorService.class);
when(executorService.isShutdown()).thenReturn(true);
AtomicBoolean terminated = new AtomicBoolean();
- AtomicBoolean awaitTerminationInterrupted = new AtomicBoolean();
+ CompletableFuture<Boolean> awaitTerminationInterrupted = new CompletableFuture<>();
when(executorService.isTerminated()).thenAnswer(invocation -> terminated.get());
+ CountDownLatch awaitingTerminationEntered = new CountDownLatch(1);
when(executorService.awaitTermination(anyLong(), any())).thenAnswer(invocation -> {
long timeout = invocation.getArgument(0);
TimeUnit unit = invocation.getArgument(1);
+ awaitingTerminationEntered.countDown();
Review comment:
move `awaitingTerminationEntered.countDown();` into try block
--
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] linlinnn commented on pull request #10599: [Tests] Fix flaky test GracefulExecutorServicesShutdownTest
Posted by GitBox <gi...@apache.org>.
linlinnn commented on pull request #10599:
URL: https://github.com/apache/pulsar/pull/10599#issuecomment-841802478
@lhotari
Oh, I make a wrong understand with the outer catch InterruptedException in `GracefulExecutorServicesTerminationHandler#awaitTermination` since executor.awaitTermination has been mocked.
Sorry for my mistake.
--
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 #10599: [Tests] Fix flaky test GracefulExecutorServicesShutdownTest
Posted by GitBox <gi...@apache.org>.
lhotari commented on a change in pull request #10599:
URL: https://github.com/apache/pulsar/pull/10599#discussion_r633049871
##########
File path: pulsar-broker/src/test/java/org/apache/pulsar/broker/service/GracefulExecutorServicesShutdownTest.java
##########
@@ -120,25 +121,28 @@ public void shouldWaitForExecutorToTerminate() throws ExecutionException, Interr
@Test
- public void shouldTerminateWhenFutureIsCancelled() throws InterruptedException {
+ public void shouldTerminateWhenFutureIsCancelled() throws InterruptedException, ExecutionException {
// given
GracefulExecutorServicesShutdown shutdown = GracefulExecutorServicesShutdown.initiate();
shutdown.timeout(Duration.ofMillis(15000));
ExecutorService executorService = mock(ExecutorService.class);
when(executorService.isShutdown()).thenReturn(true);
AtomicBoolean terminated = new AtomicBoolean();
- AtomicBoolean awaitTerminationInterrupted = new AtomicBoolean();
+ CompletableFuture<Boolean> awaitTerminationInterrupted = new CompletableFuture<>();
when(executorService.isTerminated()).thenAnswer(invocation -> terminated.get());
+ CountDownLatch awaitingTerminationEntered = new CountDownLatch(1);
when(executorService.awaitTermination(anyLong(), any())).thenAnswer(invocation -> {
long timeout = invocation.getArgument(0);
TimeUnit unit = invocation.getArgument(1);
+ awaitingTerminationEntered.countDown();
Review comment:
What caused the flakiness was that this condition caused the execution to never get to the Thread.sleep when the thread was already interrupted:
https://github.com/apache/pulsar/blob/a1cebdb3e7cc3b8ee3ce567058182e7d31c762d2/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/GracefulExecutorServicesTerminationHandler.java#L112
This is fixed with the use of the CountDownLatch.
--
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] linlinnn commented on pull request #10599: [Tests] Fix flaky test GracefulExecutorServicesShutdownTest
Posted by GitBox <gi...@apache.org>.
linlinnn commented on pull request #10599:
URL: https://github.com/apache/pulsar/pull/10599#issuecomment-841785603
--
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 #10599: [Tests] Fix flaky test GracefulExecutorServicesShutdownTest
Posted by GitBox <gi...@apache.org>.
lhotari commented on a change in pull request #10599:
URL: https://github.com/apache/pulsar/pull/10599#discussion_r633049871
##########
File path: pulsar-broker/src/test/java/org/apache/pulsar/broker/service/GracefulExecutorServicesShutdownTest.java
##########
@@ -120,25 +121,28 @@ public void shouldWaitForExecutorToTerminate() throws ExecutionException, Interr
@Test
- public void shouldTerminateWhenFutureIsCancelled() throws InterruptedException {
+ public void shouldTerminateWhenFutureIsCancelled() throws InterruptedException, ExecutionException {
// given
GracefulExecutorServicesShutdown shutdown = GracefulExecutorServicesShutdown.initiate();
shutdown.timeout(Duration.ofMillis(15000));
ExecutorService executorService = mock(ExecutorService.class);
when(executorService.isShutdown()).thenReturn(true);
AtomicBoolean terminated = new AtomicBoolean();
- AtomicBoolean awaitTerminationInterrupted = new AtomicBoolean();
+ CompletableFuture<Boolean> awaitTerminationInterrupted = new CompletableFuture<>();
when(executorService.isTerminated()).thenAnswer(invocation -> terminated.get());
+ CountDownLatch awaitingTerminationEntered = new CountDownLatch(1);
when(executorService.awaitTermination(anyLong(), any())).thenAnswer(invocation -> {
long timeout = invocation.getArgument(0);
TimeUnit unit = invocation.getArgument(1);
+ awaitingTerminationEntered.countDown();
Review comment:
What caused the flakiness was that this condition caused the execution to never get to the Thread.sleep when the thread was already interrupted:
https://github.com/apache/pulsar/blob/a1cebdb3e7cc3b8ee3ce567058182e7d31c762d2/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/GracefulExecutorServicesTerminationHandler.java#L112
This is fixed with the use of the CountDownLatch.
##########
File path: pulsar-broker/src/test/java/org/apache/pulsar/broker/service/GracefulExecutorServicesShutdownTest.java
##########
@@ -120,25 +121,28 @@ public void shouldWaitForExecutorToTerminate() throws ExecutionException, Interr
@Test
- public void shouldTerminateWhenFutureIsCancelled() throws InterruptedException {
+ public void shouldTerminateWhenFutureIsCancelled() throws InterruptedException, ExecutionException {
// given
GracefulExecutorServicesShutdown shutdown = GracefulExecutorServicesShutdown.initiate();
shutdown.timeout(Duration.ofMillis(15000));
ExecutorService executorService = mock(ExecutorService.class);
when(executorService.isShutdown()).thenReturn(true);
AtomicBoolean terminated = new AtomicBoolean();
- AtomicBoolean awaitTerminationInterrupted = new AtomicBoolean();
+ CompletableFuture<Boolean> awaitTerminationInterrupted = new CompletableFuture<>();
when(executorService.isTerminated()).thenAnswer(invocation -> terminated.get());
+ CountDownLatch awaitingTerminationEntered = new CountDownLatch(1);
when(executorService.awaitTermination(anyLong(), any())).thenAnswer(invocation -> {
long timeout = invocation.getArgument(0);
TimeUnit unit = invocation.getArgument(1);
+ awaitingTerminationEntered.countDown();
Review comment:
I don't see a reason to move `awaitingTerminationEntered.countDown();` into the try block since that doesn't make a difference.
--
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 pull request #10599: [Tests] Fix flaky test GracefulExecutorServicesShutdownTest
Posted by GitBox <gi...@apache.org>.
lhotari commented on pull request #10599:
URL: https://github.com/apache/pulsar/pull/10599#issuecomment-841823316
> Oh, I make a wrong understand with the outer catch InterruptedException in `GracefulExecutorServicesTerminationHandler#awaitTermination` since executor.awaitTermination has been mocked.
> Sorry for my mistake.
No worries, the mocking was quite unexpected for executor.awaitTermination.
btw. It seems that this test became very flaky after the switch to JDK11. To get the fix to other PRs, it is either necessary to rebase or close and re-open a PR so that the changes get picked up. When re-running a failed build it won't pick up changes made into master branch. New PR builds or reopened PR builds will pick up 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] lhotari commented on a change in pull request #10599: [Tests] Fix flaky test GracefulExecutorServicesShutdownTest
Posted by GitBox <gi...@apache.org>.
lhotari commented on a change in pull request #10599:
URL: https://github.com/apache/pulsar/pull/10599#discussion_r633049980
##########
File path: pulsar-broker/src/test/java/org/apache/pulsar/broker/service/GracefulExecutorServicesShutdownTest.java
##########
@@ -120,25 +121,28 @@ public void shouldWaitForExecutorToTerminate() throws ExecutionException, Interr
@Test
- public void shouldTerminateWhenFutureIsCancelled() throws InterruptedException {
+ public void shouldTerminateWhenFutureIsCancelled() throws InterruptedException, ExecutionException {
// given
GracefulExecutorServicesShutdown shutdown = GracefulExecutorServicesShutdown.initiate();
shutdown.timeout(Duration.ofMillis(15000));
ExecutorService executorService = mock(ExecutorService.class);
when(executorService.isShutdown()).thenReturn(true);
AtomicBoolean terminated = new AtomicBoolean();
- AtomicBoolean awaitTerminationInterrupted = new AtomicBoolean();
+ CompletableFuture<Boolean> awaitTerminationInterrupted = new CompletableFuture<>();
when(executorService.isTerminated()).thenAnswer(invocation -> terminated.get());
+ CountDownLatch awaitingTerminationEntered = new CountDownLatch(1);
when(executorService.awaitTermination(anyLong(), any())).thenAnswer(invocation -> {
long timeout = invocation.getArgument(0);
TimeUnit unit = invocation.getArgument(1);
+ awaitingTerminationEntered.countDown();
Review comment:
I don't see a reason to move `awaitingTerminationEntered.countDown();` into the try block since that doesn't make a difference.
--
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 #10599: [Tests] Fix flaky test GracefulExecutorServicesShutdownTest
Posted by GitBox <gi...@apache.org>.
eolivelli commented on pull request #10599:
URL: https://github.com/apache/pulsar/pull/10599#issuecomment-841788102
@linlinnn I have merged this patch in order to unblock CI.
We can continue the discussion and find a better fix if think that this is not the best way
--
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 #10599: [Tests] Fix flaky test GracefulExecutorServicesShutdownTest
Posted by GitBox <gi...@apache.org>.
lhotari commented on a change in pull request #10599:
URL: https://github.com/apache/pulsar/pull/10599#discussion_r632971688
##########
File path: pulsar-broker/src/test/java/org/apache/pulsar/broker/service/GracefulExecutorServicesShutdownTest.java
##########
@@ -120,25 +121,28 @@ public void shouldWaitForExecutorToTerminate() throws ExecutionException, Interr
@Test
- public void shouldTerminateWhenFutureIsCancelled() throws InterruptedException {
+ public void shouldTerminateWhenFutureIsCancelled() throws InterruptedException, ExecutionException {
// given
GracefulExecutorServicesShutdown shutdown = GracefulExecutorServicesShutdown.initiate();
shutdown.timeout(Duration.ofMillis(15000));
ExecutorService executorService = mock(ExecutorService.class);
when(executorService.isShutdown()).thenReturn(true);
AtomicBoolean terminated = new AtomicBoolean();
- AtomicBoolean awaitTerminationInterrupted = new AtomicBoolean();
+ CompletableFuture<Boolean> awaitTerminationInterrupted = new CompletableFuture<>();
when(executorService.isTerminated()).thenAnswer(invocation -> terminated.get());
+ CountDownLatch awaitingTerminationEntered = new CountDownLatch(1);
when(executorService.awaitTermination(anyLong(), any())).thenAnswer(invocation -> {
long timeout = invocation.getArgument(0);
TimeUnit unit = invocation.getArgument(1);
+ awaitingTerminationEntered.countDown();
Review comment:
> move `awaitingTerminationEntered.countDown();` into try block
Does that make a difference?
--
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 merged pull request #10599: [Tests] Fix flaky test GracefulExecutorServicesShutdownTest
Posted by GitBox <gi...@apache.org>.
eolivelli merged pull request #10599:
URL: https://github.com/apache/pulsar/pull/10599
--
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 #10599: [Tests] Fix flaky test GracefulExecutorServicesShutdownTest
Posted by GitBox <gi...@apache.org>.
eolivelli commented on pull request #10599:
URL: https://github.com/apache/pulsar/pull/10599#issuecomment-841788102
@linlinnn I have merged this patch in order to unblock CI.
We can continue the discussion and find a better fix if think that this is not the best way
--
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 merged pull request #10599: [Tests] Fix flaky test GracefulExecutorServicesShutdownTest
Posted by GitBox <gi...@apache.org>.
eolivelli merged pull request #10599:
URL: https://github.com/apache/pulsar/pull/10599
--
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 pull request #10599: [Tests] Fix flaky test GracefulExecutorServicesShutdownTest
Posted by GitBox <gi...@apache.org>.
lhotari commented on pull request #10599:
URL: https://github.com/apache/pulsar/pull/10599#issuecomment-841793435
--
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