add configs to enable fast request failure on broker and historical by himanshug · Pull Request #4540 · apache/druid

himanshug · 2017-07-13T15:54:27Z

Currently clients can overwhelm a broker inadvertently by sending too many requests which get queued in unbounded jetty worker pool queue. Clients typically close the connection after a certain client side timeout but Broker has to handle all those requests giving appearance of being unresponsive while client would continue to retry and be stuck in this endless loop.

This patch provides configurations to have two type of limits.

(1) ability to limit jetty queue size: to be used when user wants to have limited number of requests waiting when all threads are busy handling other requests.
(2) ability to disable queuing: to be used when user want no request to wait but rather be rejected immediately.

See doc updates to broker.md and historical.md for configuration details.

drcrallen

Good addition, but I have some architectural questions on how this is being applied.

drcrallen · 2017-07-13T16:22:39Z

-    threadPool.setMinThreads(config.getNumThreads());
-    threadPool.setMaxThreads(config.getNumThreads());
+    final QueuedThreadPool threadPool;
+    if (config.getQueueSize() == Integer.MAX_VALUE) {


Can this reference a static constant in the io.druid package tree somewhere?

i think its fine given that its a static constant already and just really checking whether queue is unbounded or bounded.

drcrallen · 2017-07-13T16:22:56Z

+      threadPool = new QueuedThreadPool(
+          config.getNumThreads(),
+          config.getNumThreads(),
+          60000,


Does this need to be hard coded?

yes, just the default value that is used when you do new QueuedThreadPool() in current code, it isn't/wasn't configurable and i don't think it needs to be.
later socket idle timeout is set which is independent of this and that is indeed configurable.

drcrallen · 2017-07-13T16:28:27Z

+  @Override
+  public Filter getFilter()
+  {
+    return new LimitRequestsFilter(maxActiveRequests);


the usage in io.druid.server.initialization.jetty.JettyServerInitUtils#addExtensionFilters seems to indicate this is expected to be memoized, but none of the implementations treat it that way. Is this a minor mis-handling in the Jetty Server Init Utils?

'io.druid.server.initialization.jetty.JettyServerInitUtils#addExtensionFilters' gets called only once in the lifetime of Druid process when setting up Jetty , so getFilter() also gets called just once. It doesn't matter whether its memoized or not.

drcrallen · 2017-07-13T16:28:55Z

+  {
+    private final int maxActiveRequests;
+
+    private AtomicInteger activeRequestsCount = new AtomicInteger();


suggest final.

drcrallen · 2017-07-13T16:29:55Z

+          chain.doFilter(request, response);
+        } else {
+          // See https://tools.ietf.org/html/rfc6585 for status code 429 explanation.
+          ((HttpServletResponse)response).sendError(429, "Too Many Requests");


Will this actually stop the response? if another filter attempts to send a DIFFERENT response, it is not at all clear which one would win. Especially since the Set usage of servlet holders makes the ordering non deterministic.

guice doc says that set iteration order is same as the order of inserts done ( https://google.github.io/guice/api-docs/4.1/javadoc/com/google/inject/multibindings/Multibinder.html )
however its not guaranteed that this filter will be the first in the chain but it will definitely appear before any user filters defined in extensions.

now this will work as long as all filters before this filter follow the general rule of never blocking/holding the request .

however, as I think about it and to be absolutely sure.. I am going to move the add of this filter inside QueryJettyServerInitializer directly instead of using the filter holder mechanism , then its place in the chain would be guaranteed. That would have a side effect of this feature being technically available on historicals as well but that shouldn't change anything given that by default it wouldn't be included in the filter chain.

makes sense ?

himanshug · 2017-07-24T21:47:23Z

@drcrallen ?

niketh · 2017-07-27T07:09:52Z

@himanshug Is there any advantage of having the queueSize variable? We are throttling on the activeRequests.

We can actually have queue size as the default (unbounded) and throttle on the activeRequests like you have done. This way we will avoid having yet another config variable for tuning also

himanshug · 2017-07-31T20:00:59Z

@niketh bounding the queueSize is available option for safety only , just in case if you didn't have enough jetty threads to handle requests before failing at the filter.

himanshug · 2017-09-13T02:02:54Z

@drcrallen ping ?

drcrallen · 2017-10-05T18:12:40Z

    }
-    catch (ConfigurationException e) {
-      throw new ProvisionException(Iterables.getFirst(e.getErrorMessages(), null).getMessage());
+    catch (Exception e) {


This will swallow interrupted exceptions, is it needed to catch all exceptions here?

Also, is there a reason the ProvisionException wouldn't be able to just take a "caused by"?

at this point server startup would fail so it doesn't matter as we would do same for interrupted exception.

and, no ProvidsionException does not take caused by.

What's special about ProvisionException? Losing the actual stacktrace of an exception generally sucks (especially if this is going to kill the process start). Can we just wrap into a RuntimeException instead?
[Himanshu] changed, that works too.

drcrallen · 2017-10-05T18:14:37Z

+          serverConfig.getNumThreads() > 1,
+          "numThreads must be > 1 to enable Request Limit Filter."
+      );
+      log.info("Enabling Request Limit Filter with limit [%d].", serverConfig.getNumThreads()-1);


if you have x threads to handle request concurrently, then you can allow only "x-1" to go through the filter.. xth request needs to be rejected because (x+1)th request wouldn't come to filter and would rather wait on jetty queue.

Ah, my brain had this as processing threads, but its actually jetty threads. gotcha

added it as a comment.

drcrallen · 2017-10-05T18:15:27Z

+      );
+      log.info("Enabling Request Limit Filter with limit [%d].", serverConfig.getNumThreads()-1);
+      root.addFilter(new FilterHolder(new LimitRequestsFilter(serverConfig.getNumThreads()-1)),
+                     "/*", null


Some of the "management" endpoints (like lookups) have a thread limit already. Having them both collide could be confusing.

not sure about that but it should be simple to understand. this is a server level hard limit.... finer end point specific limits are there for resource appropriation really.

drcrallen · 2017-10-05T18:17:52Z

io.druid.server.initialization.jetty.JettyBindings#addQosFilter has some similar functionality, is there any particular reason a similar approach would not be used here?

For example, do you really need to specify the atomic counter in order to achieve what you want?

himanshug · 2017-10-05T18:21:56Z

@drcrallen QoSFilter can probably be used but looked way more complex to me to handle this case of applying a hard limit , LimitRequestsFilter does least amount of work and is very very fast. I chose to write it given that its being used for all the incoming requests.

drcrallen

Suggest adding some unit tests for the new classes but otherwise looks good!

cheddar

I'm approving, but my comment asking for more clarity in the comment I think should be addressed before merging. I'm approving because I think it is fine to merge once addressed without further review.

cheddar · 2017-10-05T20:59:28Z

 */
 public class JettyServerModule extends JerseyServletModule
 {
+  // This is the maximum number of threads used by jetty acceptors and selectors


Given that this is a magic number based on the current implementation of Jetty that we use, please specify the breakdown of the magic number and the version of Jetty is it based on.

added more details on computed number, also made it compute based on number of ServerConnectors being setup (we know that based on settings of whether plainText or TLS or both connectors are configured)

cheddar · 2017-10-05T21:01:39Z

    }
-    catch (ConfigurationException e) {
-      throw new ProvisionException(Iterables.getFirst(e.getErrorMessages(), null).getMessage());
+    catch (Exception e) {


What's special about ProvisionException? Losing the actual stacktrace of an exception generally sucks (especially if this is going to kill the process start). Can we just wrap into a RuntimeException instead?
[Himanshu] changed, that works too.

jon-wei · 2018-01-05T02:33:21Z

@himanshug I'm putting together the 0.12.0 release notes now, can you update those with some info for this PR? #5211

himanshug · 2018-01-05T17:43:20Z

@jon-wei sure, will update

himanshug · 2018-01-05T22:35:10Z

updated 0.12.0 release notes #5211

jon-wei · 2018-01-05T22:37:32Z

@himanshug I'm not seeing the update, I hope I didn't clobber your changes D: (I was updating some of the highlights)

himanshug · 2018-01-05T22:49:14Z

@jon-wei np, updated again. save a local copy this time :)

jon-wei · 2018-01-05T22:52:15Z

@himanshug great, thanks!

add configs to enable fast request failure on broker

391e615

himanshug added the Improvement label Jul 13, 2017

himanshug added this to the 0.11.0 milestone Jul 13, 2017

drcrallen requested changes Jul 13, 2017

View reviewed changes

address review comments

7058495

jon-wei modified the milestones: 0.11.0, 0.11.1 Sep 20, 2017

himanshug added 5 commits October 4, 2017 09:30

Merge remote-tracking branch 'druidio/master' into broker_rate_limit

c501806

Merge remote-tracking branch 'druidio/master' into broker_rate_limit

410bcb7

fix styling error

26e89db

fix style error

12ece52

have enableRequestLimit config instead of having user specify max limit

7dedbd6

himanshug changed the title ~~add configs to enable fast request failure on broker~~ add configs to enable fast request failure on broker and historical Oct 5, 2017

himanshug added the Release Notes label Oct 5, 2017

drcrallen reviewed Oct 5, 2017

View reviewed changes

add comment

f0509d0

drcrallen approved these changes Oct 5, 2017

View reviewed changes

himanshug added 2 commits October 5, 2017 15:20

fix style error

ea6dbe9

add UT fo LimitRequestsFilter

ff71391

cheddar approved these changes Oct 5, 2017

View reviewed changes

address review comments

1d16220

himanshug force-pushed the broker_rate_limit branch from c1e6045 to 1d16220 Compare October 5, 2017 21:23

himanshug added 4 commits October 5, 2017 19:14

fix test

57298e9

Merge remote-tracking branch 'druidio/master' into broker_rate_limit

43f4e8d

make LimitRequestsFilterTest more robust

c243607

fix JettyQosTest

2534586

himanshug merged commit 0e856ee into apache:master Oct 6, 2017

himanshug deleted the broker_rate_limit branch December 29, 2017 17:29

jon-wei mentioned this pull request Jan 5, 2018

[WIP] Druid 0.12.0 release notes #5211

Closed

Conversation

himanshug commented Jul 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

drcrallen 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!

himanshug Jul 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

himanshug commented Jul 24, 2017

Uh oh!

niketh commented Jul 27, 2017

Uh oh!

himanshug commented Jul 31, 2017

Uh oh!

himanshug commented Sep 13, 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!

cheddar Oct 5, 2017 • edited by himanshug Loading Uh oh! There was an error while loading. Please reload this page.

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!

drcrallen commented Oct 5, 2017

Uh oh!

himanshug commented Oct 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

drcrallen left a comment

Choose a reason for hiding this comment

Uh oh!

cheddar 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!

cheddar Oct 5, 2017 • edited by himanshug Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jon-wei commented Jan 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

himanshug commented Jul 13, 2017 •

edited

Loading

himanshug Jul 13, 2017 •

edited

Loading

cheddar Oct 5, 2017 •

edited by himanshug

Loading

himanshug commented Oct 5, 2017 •

edited

Loading

cheddar Oct 5, 2017 •

edited by himanshug

Loading

jon-wei commented Jan 5, 2018 •

edited

Loading