fix: If startup hangs (esp on preconditions), shutdown server correctly #4889

AlanConfluent · 2020-03-24T23:08:30Z

Description

Often when the server is starting up, it waits for preconditions to be met. If the preconditions can't be met, it tries indefinitely. If the server shuts down, the JVM will not stop due to this unending preconditions effort.

This change allows the preconditions to be interrupted during shutdown so that the application and JVM exit properly.

Fixes #4779

Testing done

mvn package

Reviewer checklist

Ensure docs are updated if necessary. (eg. if a user visible feature is being added or changed).
Ensure relevant issues are linked (description should include text like "Fixes #")

rodesai

LGTM

rodesai · 2020-03-25T06:19:50Z

How'd you test it out? Were you able to reproduce locally?

AlanConfluent · 2020-03-25T19:10:42Z

How'd you test it out? Were you able to reproduce locally?

I introduced a precondition that never completes. Then I ran the server and would then issue a kill command, which would call the shutdown method. I pretty much verified that the startup method completes immediately and that the jvm quits within some reasonable window, e.g. 5-10 seconds after that.

big-andy-coates

Sorry for the late review.

Nothing major in here, just wanted to flag up some potential issues and some suggests.

big-andy-coates · 2020-03-26T10:22:13Z

ksqldb-rest-app/src/main/java/io/confluent/ksql/rest/server/KsqlRestApplication.java

@@ -391,6 +404,13 @@ private KsqlFailedPrecondition(final String message) {
    }
  }

+  static final class AbortApplicationStartException extends Exception {


Why are we using a checked exception here? We don't really use checked exceptions any more. They're a PITA! Why not extend KsqlServerException?

Sure. Extended KsqlServerException.

big-andy-coates · 2020-03-26T10:25:09Z

ksqldb-common/src/main/java/io/confluent/ksql/util/RetryUtil.java

@@ -53,6 +55,7 @@ public static void retryWithBackoff(
      final int initialWaitMs,
      final int maxWaitMs,
      final Runnable runnable,
+      final AtomicBoolean stopRetrying,


nit: Rather than taking a AtomicBoolean, why not take a Supplier<Boolean>? The latter is more general and flexible, allowing method that returns a boolean to be used.

You can then wire this up by passing stopRetrying::get.

Good suggestion.

big-andy-coates · 2020-03-26T10:29:20Z

ksqldb-common/src/test/java/io/confluent/ksql/util/RetryUtilTest.java

+    try {
+      RetryUtil.retryWithBackoff(3, 1, 100, runnable, sleep, stopRetrying, Collections.emptyList());
+      fail("retry should have thrown");
+    } catch (final RuntimeException e) {
+    }


You can do this with less code with:

assertThrows( RuntimeException.class, () -> RetryUtil.retryWithBackoff(3, 1, 100, runnable, sleep, stopRetrying, Collections.emptyList()) );

big-andy-coates · 2020-03-26T10:31:45Z

ksqldb-rest-app/src/main/java/io/confluent/ksql/rest/server/KsqlRestApplication.java

+          predicates
+      );
+    } catch (KsqlFailedPrecondition e) {
+      log.error("Finished Precondition retrying", e);


This message seems a little confusing. It's an error, but the text sounds more like a success... maybe

log.error("Failed to meet preconditions. Existing...", e);

Yeah, I agree. Changed.

big-andy-coates · 2020-03-26T10:35:25Z

ksqldb-rest-app/src/main/java/io/confluent/ksql/rest/server/KsqlRestApplication.java

+    // startup thread if it's been hanging.
+    shuttingDown.set(true);
+    if (startAsyncThread != null) {
+      startAsyncThread.interrupt();


If we're interrupting the start up thread, then there's a high chance we'll see interrupt exceptions being thrown. Yet I don't see anywhere that we catch this, now expected, exception. This may lead to the exception being logged out, which isn't ideal if its expected.

Maybe the code in startAsync need to also catch interrupted exceptions and log out the Aborting application start?

In this particular case, it's caught in RetryUtil:

duration -> { try { Thread.sleep(duration); } catch (final InterruptedException e) { log.debug("retryWithBackoff interrupted while sleeping"); } },

I believe it would only throw this exception if someone did something that could be interrupted (like sleeping), and since it's a checked exception, they would have to catch it explicitly (or let it bubble up explicitly). In this case, I would want them to catch it and then throw a AbortApplicationStartException which is already handled. So I don't think catching InterruptedExceptions at the startAsync level would be required. Is that reasonable?

big-andy-coates · 2020-03-26T10:35:55Z

ksqldb-rest-app/src/test/java/io/confluent/ksql/rest/server/KsqlRestApplicationTest.java

@@ -415,7 +421,7 @@ public void shouldNotInitializeUntilPreconditionsChecked() {
  }

  @Test
-  public void shouldConfigureRocksDBConfigSetter() {
+  public void shouldConfigureRocksDBConfigSetter() throws AbortApplicationStartException {


If we make AbortApplicationStartException a non-checked exception we can drop all these throws...

AlanConfluent

@big-andy-coates Replied to all of your comments:

#4907

AlanConfluent · 2020-03-26T20:51:24Z

ksqldb-rest-app/src/main/java/io/confluent/ksql/rest/server/KsqlRestApplication.java

@@ -391,6 +404,13 @@ private KsqlFailedPrecondition(final String message) {
    }
  }

+  static final class AbortApplicationStartException extends Exception {


Sure. Extended KsqlServerException.

AlanConfluent · 2020-03-26T20:53:08Z

ksqldb-rest-app/src/main/java/io/confluent/ksql/rest/server/KsqlRestApplication.java

+          predicates
+      );
+    } catch (KsqlFailedPrecondition e) {
+      log.error("Finished Precondition retrying", e);


Yeah, I agree. Changed.

AlanConfluent · 2020-03-26T21:03:23Z

ksqldb-rest-app/src/main/java/io/confluent/ksql/rest/server/KsqlRestApplication.java

+    // startup thread if it's been hanging.
+    shuttingDown.set(true);
+    if (startAsyncThread != null) {
+      startAsyncThread.interrupt();


In this particular case, it's caught in RetryUtil:

duration -> { try { Thread.sleep(duration); } catch (final InterruptedException e) { log.debug("retryWithBackoff interrupted while sleeping"); } },

I believe it would only throw this exception if someone did something that could be interrupted (like sleeping), and since it's a checked exception, they would have to catch it explicitly (or let it bubble up explicitly). In this case, I would want them to catch it and then throw a AbortApplicationStartException which is already handled. So I don't think catching InterruptedExceptions at the startAsync level would be required. Is that reasonable?

AlanConfluent · 2020-03-26T21:03:39Z

ksqldb-rest-app/src/test/java/io/confluent/ksql/rest/server/KsqlRestApplicationTest.java

@@ -415,7 +421,7 @@ public void shouldNotInitializeUntilPreconditionsChecked() {
  }

  @Test
-  public void shouldConfigureRocksDBConfigSetter() {
+  public void shouldConfigureRocksDBConfigSetter() throws AbortApplicationStartException {


AlanConfluent · 2020-03-26T21:04:10Z

ksqldb-common/src/main/java/io/confluent/ksql/util/RetryUtil.java

@@ -53,6 +55,7 @@ public static void retryWithBackoff(
      final int initialWaitMs,
      final int maxWaitMs,
      final Runnable runnable,
+      final AtomicBoolean stopRetrying,


Good suggestion.

AlanConfluent · 2020-03-26T21:11:24Z

ksqldb-common/src/test/java/io/confluent/ksql/util/RetryUtilTest.java

+    try {
+      RetryUtil.retryWithBackoff(3, 1, 100, runnable, sleep, stopRetrying, Collections.emptyList());
+      fail("retry should have thrown");
+    } catch (final RuntimeException e) {
+    }


fix: If startup hangs (esp on preconditions), shutdown server correctly

bc2092c

AlanConfluent requested a review from rodesai March 24, 2020 23:08

AlanConfluent requested a review from a team as a code owner March 24, 2020 23:08

rodesai approved these changes Mar 25, 2020

View reviewed changes

AlanConfluent merged commit 20c4b59 into confluentinc:master Mar 25, 2020

big-andy-coates reviewed Mar 26, 2020

View reviewed changes

AlanConfluent mentioned this pull request Mar 26, 2020

refactor: Followup comments to PR 4889 #4907

Merged

2 tasks

AlanConfluent commented Mar 26, 2020

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: If startup hangs (esp on preconditions), shutdown server correctly #4889

fix: If startup hangs (esp on preconditions), shutdown server correctly #4889

AlanConfluent commented Mar 24, 2020

rodesai left a comment

rodesai commented Mar 25, 2020

AlanConfluent commented Mar 25, 2020

big-andy-coates left a comment

big-andy-coates Mar 26, 2020

AlanConfluent Mar 26, 2020

big-andy-coates Mar 26, 2020

AlanConfluent Mar 26, 2020

big-andy-coates Mar 26, 2020

AlanConfluent Mar 26, 2020

big-andy-coates Mar 26, 2020

AlanConfluent Mar 26, 2020

big-andy-coates Mar 26, 2020

AlanConfluent Mar 26, 2020

big-andy-coates Mar 26, 2020

AlanConfluent Mar 26, 2020

AlanConfluent left a comment

AlanConfluent Mar 26, 2020

AlanConfluent Mar 26, 2020

AlanConfluent Mar 26, 2020

AlanConfluent Mar 26, 2020

AlanConfluent Mar 26, 2020

AlanConfluent Mar 26, 2020

fix: If startup hangs (esp on preconditions), shutdown server correctly #4889

fix: If startup hangs (esp on preconditions), shutdown server correctly #4889

Conversation

AlanConfluent commented Mar 24, 2020

Description

Testing done

Reviewer checklist

rodesai left a comment

Choose a reason for hiding this comment

rodesai commented Mar 25, 2020

AlanConfluent commented Mar 25, 2020

big-andy-coates left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlanConfluent left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment