Fix Router race condition and use default broker service name for invalid priority by jisookim0513 · Pull Request #5050 · apache/druid

jisookim0513 · 2017-11-07T01:52:30Z

This PR suggests two changes:

I believe there's a race condition in TieredBrokerHostSelector, specifically in NodeHolder.pick() where multiple threads can access roundRobinIndex. For example, two threads can simultaneously access roundRobinIndex when roundRobinIndex == currNodes.size() - 1, but then one of the threads will get index out of bounds error when calling currNodes.get(roundRobinIndex++) because the other thread had increased the value of roundRobinIndex by 1 already. I have encountered this situation in production, for example:

 WARN [qtp658824366-145] o.e.j.s.ServletHandler - /druid/v2/ - []
java.lang.IndexOutOfBoundsException: index (x) must be less than size (x)
    at com.google.common.base.Preconditions.checkElementIndex(Preconditions.java:313) ~[guava-16.0.1.jar:?]
    at com.google.common.base.Preconditions.checkElementIndex(Preconditions.java:295) ~[guava-16.0.1.jar:?]
    at com.google.common.collect.RegularImmutableList.get(RegularImmutableList.java:65) ~[guava-16.0.1.jar:?]
    at io.druid.server.router.TieredBrokerHostSelector$NodesHolder.pick(TieredBrokerHostSelector.java:319) ~[druid-server-0.11.0-mmx19.jar:0.11.0-mmx19]
    at io.druid.server.router.TieredBrokerHostSelector.getDefaultLookup(TieredBrokerHostSelector.java:260) ~[druid-server-0.11.0-mmx19.jar:0.11.0-mmx19]
    at io.druid.server.router.QueryHostFinder.findDefaultServer(QueryHostFinder.java:60) ~[druid-server-0.11.0-mmx19.jar:0.11.0-mmx19]
    at io.druid.server.router.QueryHostFinder.getDefaultServer(QueryHostFinder.java:90) ~[druid-server-0.11.0-mmx19.jar:0.11.0-mmx19]
    at io.druid.server.AsyncQueryForwardingServlet.service(AsyncQueryForwardingServlet.java:180) ~[druid-server-0.11.0-mmx19.jar:0.11.0-mmx19]
    at javax.servlet.http.HttpServlet.service(HttpServlet.java:790) ~[javax.servlet-api-3.1.0.jar:3.1.0]

I think it might make more sense to use DefaultBrokerServiceName instead of the last value of tierToBrokerMap when the priority is out of range, as I understand that the default value is the one that's used whenever there's no specific broker service name for the given query. What is the reason behind using the last entry of tierToBrokerMap when the priority is out of range?

jihoonson

Looks good to me. Left a trivial comment. +1 after travis.

jihoonson · 2017-11-07T02:03:05Z

-              tierConfig.getTierToBrokerMap().values(),
-              tierConfig.getDefaultBrokerServiceName()
-          )
+          tierConfig.getDefaultBrokerServiceName()


Logging here might be helpful.

TieredBrokerSelectorStrategy doesn't have a logger. Should I add a logger to this class?

is the log for queries which don't fall in min-max priority range configured ? that log might end up being too noisy. in many clusters, cluster operators and user teams are different and users might send such queries.

leventov · 2017-11-07T02:07:20Z

@@ -50,18 +49,10 @@ public Optional<String> getBrokerServiceName(TieredBrokerConfig tierConfig, Quer

    if (priority < minPriority) {


Two branches of this if block are the same now.

combined the two branches into one

leventov · 2017-11-07T02:07:35Z

-          )
+          tierConfig.getDefaultBrokerServiceName()
      );
    } else if (priority >= maxPriority) {


Not sure if it should be >= or >

hmm, I guess it should be >?

i'm fine with `>'

leventov · 2017-11-07T02:19:43Z

    {
      ImmutableList<Server> currNodes = nodes;

+      int index = roundRobinIndex.getAndIncrement();


It will produce negative results when index overflows, suggested to replace with a method which wraps around currNodes.size() explicitly, with use of compareAndSet(), for sanity

Or allow the overflow, but make sure the chosen index is positive (use absolute value).

himanshug · 2017-11-07T19:04:22Z

added 0.11.0 milestone due to race condition fix.

leventov · 2017-11-08T18:28:29Z

      }

-      return currNodes.get(roundRobinIndex++);
+      return currNodes.get(index);


Now this index could be incorrect, because it was set during a pervious call of this method.

I added index %= currNodes.size() back to fix this, is there a better way to do it?

I suggest to return to this version: ab61f83 and just use nextIndex instead of index below in the code, it's not a problem if indexing will starts from 1 instead of 0. Or the initial value in roundRobinIndex could be set -1.

Ok, I wasn't really sure about using nextIndex (for the name's sake) but I will revert the change to that and set the initial value of roundRobinIndex to -1.

leventov · 2017-11-08T19:23:05Z

+
+      while (true) {
+        int nextIndex = index + 1;
+        if (nextIndex < 0) nextIndex %= currNodes.size();


It won't help, the result of mod operation when the first argument is negative is zero or negative

leventov · 2017-11-08T20:07:18Z

+      int index = roundRobinIndex.get();
+      int nextIndex = index + 1;
+
+      while (true) {


Could you please extract this block as a method, it will allow to not repeat

int index = roundRobinIndex.get(); int nextIndex = index + 1;

block

leventov · 2017-11-08T20:07:30Z

+      int nextIndex = index + 1;
+
+      while (true) {
+        if (nextIndex < 0 || nextIndex >= currNodes.size()) {


nextIndex < 0 is always false

ok, will remove that

himanshug · 2017-11-08T21:46:24Z

👍

gianm · 2017-11-09T00:23:57Z

CI failed because http://www.us.apache.org/dist/zookeeper/zookeeper-3.4.6/zookeeper-3.4.6.tar.gz is no longer available. #5060 includes a fix.

…tion-and-stuff

…alid priority (apache#5050) * use default brokerServiceName when priority is not valid * use AtomicInteger for NodesHolder.roundRobinIndex * revert inspectionProfiles change * adjust TieredBrokerHostSelectorTest * combine if statements and ensure index does not become negative * set next index with mod if overflows * fix codestyle * use nextIndex * extract the while loop to a method

…alid priority (#5050) (#5066) * use default brokerServiceName when priority is not valid * use AtomicInteger for NodesHolder.roundRobinIndex * revert inspectionProfiles change * adjust TieredBrokerHostSelectorTest * combine if statements and ensure index does not become negative * set next index with mod if overflows * fix codestyle * use nextIndex * extract the while loop to a method

…alid priority (apache#5050) (apache#5066) * use default brokerServiceName when priority is not valid * use AtomicInteger for NodesHolder.roundRobinIndex * revert inspectionProfiles change * adjust TieredBrokerHostSelectorTest * combine if statements and ensure index does not become negative * set next index with mod if overflows * fix codestyle * use nextIndex * extract the while loop to a method

jisookim0513 added 4 commits November 6, 2017 17:36

use default brokerServiceName when priority is not valid

dc924b4

use AtomicInteger for NodesHolder.roundRobinIndex

91ba66a

revert inspectionProfiles change

1d7040a

adjust TieredBrokerHostSelectorTest

e0e158d

jihoonson approved these changes Nov 7, 2017

View reviewed changes

leventov reviewed Nov 7, 2017

View reviewed changes

leventov added Bug Area - Router labels Nov 7, 2017

himanshug added this to the 0.11.0 milestone Nov 7, 2017

combine if statements and ensure index does not become negative

ab61f83

leventov reviewed Nov 8, 2017

View reviewed changes

set next index with mod if overflows

77dadfd

leventov reviewed Nov 8, 2017

View reviewed changes

jisookim0513 added 2 commits November 8, 2017 11:29

fix codestyle

b72b68b

use nextIndex

4ceb339

leventov reviewed Nov 8, 2017

View reviewed changes

extract the while loop to a method

7312561

leventov approved these changes Nov 8, 2017

View reviewed changes

Merge remote-tracking branch 'upstream/master' into router-race-condi…

3b3af11

…tion-and-stuff

himanshug merged commit 1bf253f into apache:master Nov 9, 2017

drcrallen deleted the router-race-condition-and-stuff branch November 9, 2017 17:42

gianm mentioned this pull request Nov 9, 2017

[Backport] Fix Router race condition and use default broker service name for invalid priority #5066

Merged

		@@ -50,18 +49,10 @@ public Optional<String> getBrokerServiceName(TieredBrokerConfig tierConfig, Quer

		if (priority < minPriority) {

Conversation

jisookim0513 commented Nov 7, 2017

Uh oh!

jihoonson left a comment • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

himanshug Nov 7, 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!

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 commented Nov 7, 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!

Choose a reason for hiding this comment

Uh oh!

himanshug commented Nov 8, 2017

Uh oh!

gianm commented Nov 9, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

jihoonson left a comment •

edited

Loading

himanshug Nov 7, 2017 •

edited

Loading