[FLINK-7880][QS] Wait for proper resource cleanup after each ITCase. #5062

kl0u · 2017-11-23T14:09:56Z

R @aljoscha

kl0u · 2017-11-24T09:14:51Z

This is a follow-up to #4993

zentol · 2017-11-29T13:17:44Z

Your follow-up reference points to this very PR :P

kl0u · 2017-11-29T13:19:55Z

Fixed @zentol !

zentol

Review for the first 3 commits.

zentol · 2017-11-29T13:28:17Z

...ueryable-state-client-java/src/main/java/org/apache/flink/queryablestate/network/Client.java

+			});
+
+			// check again if in the meantime another thread completed the future
+			if (clientShutdownFuture.compareAndSet(null, newShutdownFuture)) {


This can never be true. We can only arrive here if clientShutdownFuture.compareAndSet(null, newShutdownFuture) succeeds, and there is no other code path that ever sets it to null again.

You are right!

zentol · 2017-11-29T13:28:52Z

...ueryable-state-client-java/src/main/java/org/apache/flink/queryablestate/network/Client.java

 					// don't want any lingering connections after shut down,
 					// which can happen if we don't check this here.
-					if (shutDown.get()) {
+					if (!clientShutdownFuture.compareAndSet(null, null)) {


for clarity we should call get() instead of compareAndSet since the setting part is a no-op anyway.

zentol · 2017-11-29T13:32:48Z

...ueryable-state-client-java/src/main/java/org/apache/flink/queryablestate/network/Client.java

 						failureCause = cause;
 					}

+					closed = true;


this field should be redundant, as (connectionShutdownFuture.get() != null) == closed should always hold.

Yes. I will remove that.

zentol · 2017-11-29T13:41:01Z

...ueryable-state-runtime/src/test/java/org/apache/flink/queryablestate/network/ClientTest.java

-	private static final FiniteDuration TEST_TIMEOUT = new FiniteDuration(10L, TimeUnit.SECONDS);
+	@Before
+	public void setUp() throws Exception {
+		nioGroup = new NioEventLoopGroup();


this method isn't strictly necessary; you can also do private final NioEventLoopGroup nioGroup = new NioEventLoopGroup(). Then you would also no longer need the null-check in @After.

zentol · 2017-11-29T13:43:38Z

...ueryable-state-runtime/src/test/java/org/apache/flink/queryablestate/network/ClientTest.java

+				} catch (Exception e) {
+					e.printStackTrace();
+				}
+				Assert.assertTrue(client.isEventGroupShutdown());


I would prefer if we would throw e in this and similar cases.

zentol · 2017-11-29T14:21:03Z

...te-client-java/src/main/java/org/apache/flink/queryablestate/network/AbstractServerBase.java

-		}
+	public CompletableFuture<Void> shutdownServer(Time timeout) throws InterruptedException {
+		CompletableFuture<Void> shutdownFuture = new CompletableFuture<>();
+		if (serverShutdownFuture.compareAndSet(null, shutdownFuture)) {


calling shutdown twice in a row will result in a future being returned that never completes.

Here the idea is that we atomically set the future the first time (serverShutdownFuture.compareAndSet(null, shutdownFuture)), and then, at each subsequent call we return that exact future (serverShutdownFuture.get();). So I cannot see how this can lead to returning a future that never completes.

zentol · 2017-11-29T14:29:04Z

...ueryable-state-client-java/src/main/java/org/apache/flink/queryablestate/network/Client.java

-					TimestampedCompletableFuture pending = pendingRequests.remove(requestId);
-					if (pending != null && pending.completeExceptionally(cause)) {
-						stats.reportFailedRequest();
+			if (connectionShutdownFuture.compareAndSet(null, shutdownFuture) &&


this can result in odd race conditions, where one thread has set the shutdownFuture, but another one the failureCause. I would suggest to create a container for the future and exception and update them atomically.

Let's also log other shutdown attempts as DEBUG.

zentol · 2017-11-29T14:33:17Z

...ueryable-state-client-java/src/main/java/org/apache/flink/queryablestate/network/Client.java

-				// requests will be handled by this failed channel.
-				establishedConnections.remove(serverAddress, this);
-			}
+			close(cause).thenAccept(cancelled -> establishedConnections.remove(serverAddress, this));


shouldn't we remove the connection in any case, since if we can't close something is probably wrong with it anyway?

zentol · 2017-11-29T14:38:09Z

...me/src/test/java/org/apache/flink/queryablestate/itcases/AbstractQueryableStateTestBase.java


-				Await.ready(cancellation, deadline.timeLeft());
+				// we are not waiting for the cancellation to happen because the
+				// job has actually failed, as tested above.


you can't guarantee this in the finally block. (for example if submitJobDetached failed but the job is actually running)

You are right. So I will just remove the finally block and check if the status of the job is FAILED.

zentol · 2017-11-29T14:38:55Z

...me/src/test/java/org/apache/flink/queryablestate/itcases/AbstractQueryableStateTestBase.java

 	 * contains a wrong jobId or wrong queryable state name.
 	 */
 	@Test
+	@Ignore


zentol · 2017-12-05T13:56:10Z

...ueryable-state-client-java/src/main/java/org/apache/flink/queryablestate/network/Client.java

 				if (throwable != null) {
-					newShutdownFuture.completeExceptionally(throwable);
-				} else if (bootstrap != null) {
+					LOG.warn("Problem while shutting down the connections at the {}: {}", clientName, throwable);


LOG.warn("Problem while shutting down the connections at the {}", clientName, throwable); instead?

zentol · 2017-12-05T14:02:40Z

...-state-runtime/src/test/java/org/apache/flink/queryablestate/network/AbstractServerTest.java


-			server2.start();
+			List<Integer> occupiedPortList = new ArrayList<>();
+			occupiedPortList.add(server1.getServerAddress().getPort());


could use Collections.singletonList() here

zentol · 2017-12-05T14:11:25Z

...me/src/test/java/org/apache/flink/queryablestate/itcases/AbstractQueryableStateTestBase.java


-	private CompletableFuture<TestingJobManagerMessages.JobStatusIs> notifyWhenJobStatusIs(
+	/**
+	 * A wrapper of the job graph that makes sure to cancell the job and wait for


typo: cancell -> cancel

zentol · 2017-12-06T12:37:24Z

...ueryable-state-runtime/src/test/java/org/apache/flink/queryablestate/network/ClientTest.java

 			assertEquals(expectedRequests, stats.getNumSuccessful());
 			assertEquals(expectedRequests, stats.getNumFailed());
 		} finally {
+			Exception exc = null;


this could be moved into the if block.

zentol · 2017-12-06T12:38:03Z

...ueryable-state-runtime/src/test/java/org/apache/flink/queryablestate/network/ClientTest.java

 				} catch (Exception e) {
-					e.printStackTrace();
+					exc = e;
+					LOG.error(ExceptionUtils.stringifyException(e));


use LOG.error("An exception occurred while shutting down netty.", e) instead

zentol · 2017-12-06T12:42:01Z

...me/src/test/java/org/apache/flink/queryablestate/itcases/AbstractQueryableStateTestBase.java

+		try {
+			final TestingJobManagerMessages.JobStatusIs jobStatus =
+					failedFuture.get(deadline.timeLeft().toMillis(), TimeUnit.MILLISECONDS);
+			assertEquals(JobStatus.FAILED, jobStatus.state());


isn't this always true if the future did not time out? (In which case get() throws a TimeoutException)

zentol · 2017-12-06T12:58:51Z

👍

kl0u · 2017-12-06T13:39:22Z

Thanks @zentol ! I will let another travis run, and then merge.

kl0u force-pushed the qs-shutdown-fin-fin branch from f7f8d2a to 43a2af6 Compare November 23, 2017 18:12

zentol reviewed Nov 29, 2017

View reviewed changes

kl0u force-pushed the qs-shutdown-fin-fin branch from 43a2af6 to 9ba9f6f Compare December 5, 2017 13:45

zentol reviewed Dec 5, 2017

View reviewed changes

kl0u force-pushed the qs-shutdown-fin-fin branch from 9ba9f6f to 7ae3e16 Compare December 6, 2017 12:34

zentol reviewed Dec 6, 2017

View reviewed changes

kl0u force-pushed the qs-shutdown-fin-fin branch from 6282a77 to e11e56a Compare December 6, 2017 12:55

kl0u force-pushed the qs-shutdown-fin-fin branch from e11e56a to 8e3c4f3 Compare December 6, 2017 13:04

kl0u added 3 commits December 6, 2017 14:33

[FLINK-7975][QS] Wait for QS client to shutdown.

5760677

[FLINK-7974][QS] Wait for QS abstract server to shutdown.

74d052b

[FLINK-7880][QS] Wait for proper resource cleanup after each ITCase.

a3fd548

kl0u force-pushed the qs-shutdown-fin-fin branch from 8e3c4f3 to a3fd548 Compare December 6, 2017 13:37

asfgit merged commit a3fd548 into apache:master Dec 6, 2017

kl0u mentioned this pull request Dec 6, 2017

[FLINK-7974][FLINK-7975][QS] Wait for shutdown in QS client and servers. #4993

Closed

kl0u deleted the qs-shutdown-fin-fin branch February 13, 2018 10:48

rmetzger added component=Runtime/QueryableState component=Tests labels Mar 18, 2019

[FLINK-7880][QS] Wait for proper resource cleanup after each ITCase. #5062

[FLINK-7880][QS] Wait for proper resource cleanup after each ITCase. #5062

Uh oh!

Conversation

kl0u commented Nov 23, 2017

Uh oh!

kl0u commented Nov 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zentol commented Nov 29, 2017

Uh oh!

kl0u commented Nov 29, 2017

Uh oh!

zentol left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zentol commented Dec 6, 2017

Uh oh!

kl0u commented Dec 6, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

kl0u commented Nov 24, 2017 •

edited

Loading