Add timeout to shutdown request to middle manager for indexing service #1862

drcrallen · 2015-10-26T18:49:50Z

If the middle manager has a catastrophic failure, the overlord can hang here while holding TaskQueue giant.lock(), causing other indexing service TaskQueue to lock up waiting for it to free..

nishantmonu51 · 2015-10-26T18:53:37Z

indexing-service/src/main/java/io/druid/indexing/overlord/RemoteTaskRunner.java

        final StatusResponseHolder response = httpClient.go(
            new Request(HttpMethod.POST, url),
            RESPONSE_HANDLER
-        ).get();
+        ).get(config.getTaskCleanupTimeout().getMillis(), TimeUnit.MILLISECONDS);


should it be taskShutdownTimeout ?

xvrl · 2015-10-26T18:56:30Z

docs/content/configuration/indexing-service.md

@@ -81,6 +81,7 @@ The following configs only apply if the overlord is running in remote mode:
 |`druid.indexer.runner.compressZnodes`|Indicates whether or not the overlord should expect middle managers to compress Znodes.|true|
 |`druid.indexer.runner.maxZnodeBytes`|The maximum size Znode in bytes that can be created in Zookeeper.|524288|
 |`druid.indexer.runner.taskCleanupTimeout`|How long to wait before failing a task after a middle manager is disconnected from Zookeeper.|PT15M|
+'`druid.indexer.runner.taskShutdownLinkTimeout`|How long to wait on a shutdown request to a middle manager before timing out|PT1M|


missing | at the beginning of the line

ha! the ' and the | look very similar on my intellij. Fixed

drcrallen · 2015-10-26T18:58:18Z

indexing-service/src/main/java/io/druid/indexing/overlord/RemoteTaskRunner.java

@@ -405,8 +406,12 @@ public void shutdown(final String taskId)
          log.error("Shutdown failed for %s! Are you sure the task was running?", taskId);
        }
      }
+      catch (InterruptedException e) {
+        Thread.currentThread().interrupt();
+        throw new RE(e, "Timed out posting shutdown to [%s] for task [%s]", url, taskId);


fixing this. incorrect message

nishantmonu51 · 2015-10-26T18:58:20Z

👍

drcrallen · 2015-10-26T19:22:18Z

@xvrl I don't think this is a regression. So it doesn't have to block 0.8.2 (even though we'll probably roll it out internally very quickly)

gianm · 2015-10-26T20:20:48Z

@drcrallen is setting the readTimeout on the http client not good enough? That's supposed to help with this kind of thing, in a nicer way- the connections get closed rather than left dangling when the future times out.

gianm · 2015-10-26T20:26:27Z

The readTimeout should also be on by default, but I think the default is highish (5 or 15 minutes maybe)

drcrallen · 2015-10-26T21:09:36Z

@gianm It would be nicer, but this uses a global http client, so setting that value would require setting the global timeout. There is no hook in the HttpClient interface to create a clone with a custom timeout. And since it uses connection pools underneath, is it possible to change timeouts for just one type of call while using the same client?

drcrallen · 2015-10-26T21:12:40Z

@gianm I can cancel/interrupt the future if that would make it cleaner.

gianm · 2015-10-26T22:00:03Z

@drcrallen I don't think the NettyHttpClient does anything special if the future is canceled.

How do you feel about using per request timeouts for this patch? this change in http-client should make it possible: metamx/http-client#24

It should also work to set the global readTimeout lower, I think the overlord doesn't do anything over http that really warrants a long timeout. The long default timeout is mostly for the broker's benefit.

drcrallen · 2015-10-26T22:12:41Z

@gianm That sounds good to me.

himanshug · 2015-10-27T18:26:58Z

indexing-service/src/main/java/io/druid/indexing/overlord/RemoteTaskRunner.java

        final StatusResponseHolder response = httpClient.go(
            new Request(HttpMethod.POST, url),
            RESPONSE_HANDLER
-        ).get();
+        ).get(config.getTaskShutdownLinkTimeout().getMillis(), TimeUnit.MILLISECONDS);


from metamx/http-client#24 , duration is supposed to go inside "go(...)" ?

Yes, will change when release of http-client is finished. (it is progressing right now)

drcrallen · 2015-10-27T20:48:19Z

@xvrl / @gianm / @himanshug should be updated now

gianm · 2015-10-27T20:49:05Z

indexing-service/src/main/java/io/druid/indexing/overlord/config/RemoteTaskRunnerConfig.java

@@ -42,6 +42,10 @@
  @Min(10 * 1024)
  private long maxZnodeBytes = 512 * 1024;

+  @JsonProperty
+  @Min(0)


Does this Min thingy work on Periods?

gooood question, probably not

removed min annotation

gianm · 2015-10-27T20:50:23Z

indexing-service/src/main/java/io/druid/indexing/overlord/RemoteTaskRunner.java

        final StatusResponseHolder response = httpClient.go(
            new Request(HttpMethod.POST, url),
-            RESPONSE_HANDLER
+            RESPONSE_HANDLER,
+            config.getTaskShutdownLinkTimeout().toStandardDuration()


would be nice to have this happen on instantiation of the Config object, because not all Periods can become Durations, and fail-fast is good.

actually since this is a rare operation, it really should happen on instantiation, as otherwise users might not notice a misconfiguration for a long time

gianm · 2015-10-27T21:02:23Z

👍

drcrallen · 2015-10-27T21:04:31Z

dumb http-client broke. fixing

Add timeout to shutdown request to middle manager for indexing service

Backport #1862

nishantmonu51 reviewed Oct 26, 2015
View reviewed changes

drcrallen added this to the 0.9.0 milestone Oct 26, 2015

drcrallen force-pushed the indexingServiceMMGone branch from 6342cc3 to 173a074 Compare October 26, 2015 18:54

xvrl reviewed Oct 26, 2015
View reviewed changes

drcrallen force-pushed the indexingServiceMMGone branch from 173a074 to 033712e Compare October 26, 2015 18:57

drcrallen reviewed Oct 26, 2015
View reviewed changes

drcrallen force-pushed the indexingServiceMMGone branch from 033712e to 2e3d5dd Compare October 26, 2015 18:59

xvrl modified the milestones: 0.8.2, 0.9.0 Oct 26, 2015

himanshug reviewed Oct 27, 2015
View reviewed changes

drcrallen force-pushed the indexingServiceMMGone branch from 2e3d5dd to 8f73736 Compare October 27, 2015 20:47

gianm reviewed Oct 27, 2015
View reviewed changes

drcrallen force-pushed the indexingServiceMMGone branch from 8f73736 to 060dc8d Compare October 27, 2015 20:49

gianm reviewed Oct 27, 2015
View reviewed changes

Add timeout to shutdown request to middle manager for indexing service

44a2b20

drcrallen force-pushed the indexingServiceMMGone branch from 060dc8d to 44a2b20 Compare October 27, 2015 20:56

Update CoordinatorRuleManager for http-client 1.0.4

67d58f6

gianm added a commit that referenced this pull request Oct 27, 2015

Merge pull request #1862 from metamx/indexingServiceMMGone

7df7370

Add timeout to shutdown request to middle manager for indexing service

gianm merged commit 7df7370 into apache:master Oct 27, 2015

drcrallen deleted the indexingServiceMMGone branch October 27, 2015 21:39

xvrl added a commit that referenced this pull request Oct 27, 2015

Merge pull request #1869 from metamx/backport1862

484a5f6

Backport #1862

drcrallen mentioned this pull request Dec 2, 2015

Make the TaskQueue big lock fair #2024

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add timeout to shutdown request to middle manager for indexing service #1862

Add timeout to shutdown request to middle manager for indexing service #1862

drcrallen commented Oct 26, 2015

nishantmonu51 Oct 26, 2015

drcrallen Oct 26, 2015

drcrallen Oct 26, 2015

xvrl Oct 26, 2015

drcrallen Oct 26, 2015

drcrallen Oct 26, 2015

nishantmonu51 commented Oct 26, 2015

drcrallen commented Oct 26, 2015

gianm commented Oct 26, 2015

gianm commented Oct 26, 2015

drcrallen commented Oct 26, 2015

drcrallen commented Oct 26, 2015

gianm commented Oct 26, 2015

drcrallen commented Oct 26, 2015

himanshug Oct 27, 2015

drcrallen Oct 27, 2015

drcrallen Oct 27, 2015

drcrallen commented Oct 27, 2015

gianm Oct 27, 2015

drcrallen Oct 27, 2015

drcrallen Oct 27, 2015

gianm Oct 27, 2015

gianm Oct 27, 2015

drcrallen Oct 27, 2015

gianm commented Oct 27, 2015

drcrallen commented Oct 27, 2015

Add timeout to shutdown request to middle manager for indexing service #1862

Add timeout to shutdown request to middle manager for indexing service #1862

Conversation

drcrallen commented Oct 26, 2015

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

nishantmonu51 commented Oct 26, 2015

drcrallen commented Oct 26, 2015

gianm commented Oct 26, 2015

gianm commented Oct 26, 2015

drcrallen commented Oct 26, 2015

drcrallen commented Oct 26, 2015

gianm commented Oct 26, 2015

drcrallen commented Oct 26, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drcrallen commented Oct 27, 2015

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

gianm commented Oct 27, 2015

drcrallen commented Oct 27, 2015