Add graceful shutdown timeout by niketh · Pull Request #4716 · apache/druid

niketh · 2017-08-24T23:33:48Z

We see queries failing while realtime handoff is happening mainly because Jetty is not being shutdown gracefully. Here is the error
Failure getting results from[http://host:port/druid/v2/] because of [org.jboss.netty.channel.ChannelException: Channel disconnected

The solution is to add a graceful shutdown. This will also reduce failed queries on shutdown on the broker and historical also.

b-slim · 2017-08-24T23:46:30Z

Thread.currentThread().interrupt(); maybe and/or rethrow

Thanks. Addressed it.

jihoonson · 2017-08-26T06:37:25Z

          public void stop()
          {
+            if (activeConnections.get() != 0) {
+              long graceFullShutDownTime = (int) server.getAttribute(GRACEFUL_SHUTDOWN_TIME);


gracefulShutDownTime?

jihoonson · 2017-08-26T06:37:53Z

+              }
+              catch (InterruptedException e) {
+                log.error("Sleep has been interrupted, while waiting for active requests to be zero");
+                Thread.currentThread().interrupt();


Should this further wait if the current thread is interrupted early?

niketh · 2017-08-29T05:49:10Z

@jihoonson Incorporated your comments

jihoonson · 2017-08-29T14:26:07Z

          {
            if (activeConnections.get() != 0) {
-              long graceFullShutDownTime = (int) server.getAttribute(GRACEFUL_SHUTDOWN_TIME);
+              long graceFulShutDownTime = (long) server.getAttribute(GRACEFUL_SHUTDOWN_TIME);


graceFul should be graceful.

jihoonson · 2017-08-29T14:28:17Z

          @Override
          public void stop()
          {
+            if (activeConnections.get() != 0) {


I think activeConnections.get() > 0 is more appropriate because we don't have to wait even if it returns a negative value due to some bugs.

jihoonson · 2017-08-29T14:42:41Z

+              }
+              catch (InterruptedException e) {
+                log.error("Sleep has been interrupted, while waiting for active requests to be zero");
+                stopImmediately();


As you added to docs, graceful shutdown is useful to wait for running queries to be completed. So, I think when an InterruptedException occurs earlier while waiting for the given GRACEFUL_SHUTDOWN_TIME, we first check activeConnections is 0 and then further wait for remaining time if there remains more active connections.
What do you think?

When an InterruptedException occurs, the thread should finish whatever it was doing as soon as possible. In this specific case , it should shutdown ASAP

Ok. Looks good to me.

gianm · 2017-09-03T20:45:41Z

  private static final Logger log = new Logger(JettyServerModule.class);

  private static final AtomicInteger activeConnections = new AtomicInteger();
+  private static final String GRACEFUL_SHUTDOWN_TIME = "graceful_shutdown_time";


minor: gracefulShutdownTimeout would be more consistent with the property name.

gianm · 2017-09-03T20:45:43Z

+              log.info("Waiting for [%s] milliseconds for active requests to be zero, current active requests=[%s]",
+                  gracefulShutDownTime, activeConnections.get());
+              try {
+                Thread.sleep(gracefulShutDownTime);


A couple questions.

Will the server always unannounce itself before entering this stop() code? How is that assured? If it happens, that's good; but if not, that would mean new requests could keep coming in continuously even while waiting for shutdown. Inevitably the last few will fail.

Could this code check activeConnections every few ms, rather than always sleeping for the gracefulShutdownTimeout? Otherwise, it looks like an extra 5s of potentially needless delay.

If both of the above are assured, then I think it could make sense to set the default gracefulShutdownTimeout to somewhat longer, like a few minutes.

gianm · 2017-09-03T20:45:55Z

          public void stop()
          {
+            if (activeConnections.get() > 0) {
+              long gracefulShutDownTime = (long) server.getAttribute(GRACEFUL_SHUTDOWN_TIME);


minor: gracefulShutdownTimeout would be more consistent with the property name.

niketh · 2017-09-06T20:44:09Z

@gianm

Servers always will announce before they shutdown
Added code to check activeConnections periodically
Increases timeout to 15s

himanshug · 2017-09-06T20:47:35Z


+  @JsonProperty
+  @NotNull
+  private Period gracefulShutdownTimeout = new Period("PT15s");


documentation says it is 5s by default. should probably be PT5s .

himanshug · 2017-09-06T20:51:23Z

+            long startTime = System.currentTimeMillis();
+            long gracefulShutDownTimeout = (long) server.getAttribute(GRACEFUL_SHUTDOWN_TIMEOUT);
+
+            while (activeConnections.get() > 0 &&


I think activeConnections is number of socket connections setup and not necessarily number of requests in progress or queued. connection pooling clients would generally never close connections and the count here might not reduce.
is there a more reliable way of looking at number of requests being processed instead ?

drcrallen · 2018-03-23T16:38:58Z

Done in #5429

niketh mentioned this pull request Aug 24, 2017

Add graceful shutdown timeout #4701

Closed

niketh force-pushed the add-shutdown-timeout branch 2 times, most recently from fda7651 to 27fcbf8 Compare August 24, 2017 23:39

Add graceful shutdown timeout

228d69b

b-slim reviewed Aug 24, 2017

View reviewed changes

Handle interruptedException

e9d329a

niketh force-pushed the add-shutdown-timeout branch from 27fcbf8 to e9d329a Compare August 25, 2017 00:01

jihoonson requested changes Aug 26, 2017

View reviewed changes

Incorporate code review comments

4f09b3f

niketh closed this Aug 29, 2017

niketh reopened this Aug 29, 2017

niketh closed this Aug 29, 2017

niketh reopened this Aug 29, 2017

niketh closed this Aug 29, 2017

niketh reopened this Aug 29, 2017

jihoonson reviewed Aug 29, 2017

View reviewed changes

Address code review comments

42a7d31

jihoonson approved these changes Aug 30, 2017

View reviewed changes

gianm reviewed Sep 3, 2017

View reviewed changes

Poll for activeConnections to be zero

127ed33

niketh force-pushed the add-shutdown-timeout branch from 57826ae to 127ed33 Compare September 5, 2017 22:37

niketh closed this Sep 6, 2017

niketh reopened this Sep 6, 2017

himanshug reviewed Sep 6, 2017

View reviewed changes

niketh force-pushed the add-shutdown-timeout branch 3 times, most recently from 374937d to e6181c4 Compare September 12, 2017 21:27

Use statistics handler to get active requests

0d07ba5

niketh force-pushed the add-shutdown-timeout branch from e6181c4 to 0d07ba5 Compare September 13, 2017 18:05

niketh closed this Sep 27, 2017

niketh reopened this Sep 27, 2017

gianm mentioned this pull request Feb 23, 2018

Exceptions in broker: Channel disconnected #5340

Closed

drcrallen mentioned this pull request Feb 27, 2018

Add graceful shutdown timeout for Jetty #5429

Merged

drcrallen closed this Mar 23, 2018

Conversation

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

Uh oh!

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!

niketh commented Aug 29, 2017

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!

niketh commented Sep 6, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

drcrallen commented Mar 23, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

niketh commented Aug 24, 2017 •

edited

Loading