add manual laning strategy, integration test by clintropolis · Pull Request #9492 · apache/druid

clintropolis · 2020-03-10T11:05:51Z

Description

This PR is part of #6993, and a follow-up to #9407 which adds a ManualLaningStrategy that in addition to being useful to make it easy to add an integration test for the query laning functionality, is also well suited for cases where one or more external applications which query Druid are able to manually decide which lane a query belongs to by adding a lane parameter to the query context.

example config:

druid.query.scheduler.laning.strategy=manual
druid.query.scheduler.laning.isLimitPercent=true
druid.query.scheduler.laning.lanes.ten=10
druid.query.scheduler.laning.lanes.twenty=20
druid.query.scheduler.laning.lanes.fifty=50

The example defines 3 lanes, "ten", "twenty", and "fifty", which if the query context contains that lane name will be allowed to use up to 10%, 20%, or 50% of the total capacity.

This PR has:

been self-reviewed.
- using the concurrency checklist (Remove this item if the PR doesn't have any relation to concurrency.)
added documentation for new or modified features or behaviors.
added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
added unit tests or modified existing tests to cover new code paths.
added integration tests.
been tested in a test Druid cluster.

maytasm

LGTM. Some comments about possible improvement in tests

maytasm · 2020-03-11T19:08:45Z

+      StatusResponseHolder status = future.get();
+      if (status.getStatus().getCode() == QueryCapacityExceededException.STATUS_CODE) {
+        limited++;
+        Assert.assertTrue(status.getContent().contains("one"));


assert this against StringUtils.format(QueryCapacityExceededException.ERROR_MESSAGE_TEMPLATE, "one") instead?

maytasm · 2020-03-11T19:11:43Z

+    Assert.assertTrue(success > 0);
+    Assert.assertTrue(limited > 0);
+
+    // test another to make sure we can still issue one query at a time


This can be separate into another @test method. Will make understanding each test case easier.

Hmm, this is testing a sequence of events though, so it belongs in the same test

I was thinking the top part is test where we exceed and expect rejection. The bottom part is test where we are always under the limit and never exceed hence expect all to be successful.

added a separate test which runs a bunch of queries the same manner that do not belong to a lane to make sure they are not limited

maytasm · 2020-03-11T19:21:31Z

+    final QueryScheduler scheduler = provider.get().get().get();
+    Assert.assertEquals(10, scheduler.getTotalAvailableCapacity());
+    Assert.assertEquals(1, scheduler.getLaneAvailableCapacity("one"));
+    Assert.assertEquals(2, scheduler.getLaneAvailableCapacity("two"));


maybe also test negative case (like lane name "three") and see we get NO_CAPACITY?

This isn't actually true currently, non-existent lanes are currently treated the same as not having a lane. I did this in #9407 because it seemed friendlier, but an argument can probably made for making undefined lanes that are present be treated as having 0 capacity which would make what you are suggesting true. If we wanted to make this change, we could either do it across the board in QueryScheduler, or leave the decision to the QueryLaningStrategy which is a bit more flexible.

Oh, I maybe interpreted this as something deeper, suggesting that we should treat queries that specify non-existent lanes as a 0 capacity lane, because of my poorly named NO_CAPACITY variable which I am going to rename as UNAVAILABLE pending this discussion. If you're just suggesting that we check that non-existent lanes return -1 I can add that test.

Im just suggesting to add test that check that non-existent lanes return -1

added some asserts to existing tests to show that non-existent lane is unavailable

maytasm · 2020-03-11T19:21:36Z

+    final QueryScheduler scheduler = provider.get().get().get();
+    Assert.assertEquals(10, scheduler.getTotalAvailableCapacity());
+    Assert.assertEquals(1, scheduler.getLaneAvailableCapacity("one"));
+    Assert.assertEquals(2, scheduler.getLaneAvailableCapacity("twenty"));


maybe also test negative case (like lane name "three") and see we get NO_CAPACITY?

maytasm · 2020-03-11T19:33:03Z

+  }
+
+  @Override
+  public <T> Optional<String> computeLane(QueryPlus<T> query, Set<SegmentServerSelector> segments)


Just wondering..why does computeLane needs SegmentServerSelector? Doesn't laning or not should just depend on the query context that was given with the query?

QueryLaningStrategy are intended to compute the lane on the query context, which the QueryScheduler can then use to enforce limits. This laning strategy is a bit of a special case because it only preserves what was already there, but other strategies could consider all sorts of information to allow them to make informed decisions about lane assignment based on details of the query.

I see. Thanks for the clarification

maytasm · 2020-03-11T19:34:47Z

+  }
+
+  @Test
+  public void testPercentLaneLimitsMustBeAboveZero()


testPercentLaneLimitsMustBeBelowOneHundred?

added more tests

maytasm · 2020-03-11T19:42:44Z

    queryHelper.testQueriesFromFile(WIKIPEDIA_QUERIES_RESOURCE, 2);
  }
+
+  @Test


Should we also have a test for when the Manual Lane From Context That Arent In Map is given? For example, if lane in query context is "some-unknown-lane"

I guess they will always be rejected since we give NO_CAPACITY right?

^ update typo above. Meant to say "also" have a test for case when the Manual Lane From Context That Arent In Map is given

jihoonson · 2020-03-12T20:48:02Z

+
+|Property|Description|Default|
+|--------|-----------|-------|
+|`druid.query.scheduler.laning.lanes.{name}`|Maximum percent or exact limit of queries that can concurrently run in the defined lanes. Any number of lanes may be defined like this.|No default, must define at least one lane with a limit above 0. If `druid.query.scheduler.laning.isLimitPercent` is set to `true`, numbers must be in the range of 1 to 100.|


numbers must be in the range of 1 to 100.

Should it say the number type is the integer?

jihoonson · 2020-03-12T21:43:56Z

+    // the broker is configured with 2 manually defined query lanes, 'one' with limit 1, and 'two' with limit 'two'
+    //  -Ddruid.query.scheduler.laning.type=manual
+    //  -Ddruid.query.scheduler.laning.lanes.one=1
+    //  -Ddruid.query.scheduler.laning.lanes.two=2


Seems the lane 'two' is not used. Perhaps better to remove it for now.

jihoonson · 2020-03-12T21:47:24Z

+    this.isLimitPercent = isLimitPercent != null ? isLimitPercent : false;
+    Preconditions.checkArgument(lanes.size() > 0, "lanes must define at least one lane");
+    Preconditions.checkArgument(
+        lanes.values().stream().allMatch(x -> this.isLimitPercent ? 0 < x && x <= 100 : x > 0),


Should it check the sum of percents of lanes is less than or equal to 100 when isLimitPercent = true?

I'm not sure it is necessary to enforce that since lane limits are currently an 'at most' guarantee instead of an 'at least'; so it seems possibly legitimate to say configure two lanes and allow either of them to use 75% of total capacity.

Hmm I see. Then should this be documented?

jihoonson

LGTM!

clintropolis · 2020-03-14T03:06:48Z

thanks for review @maytasm3 and @jihoonson

add manual laning strategy, integration test, json config test

6654a72

clintropolis added Area - Querying Area - Operations Release Notes labels Mar 10, 2020

clintropolis added 2 commits March 10, 2020 04:10

share percent conversion method

b187125

wrong assert

700e6ab

maytasm approved these changes Mar 11, 2020

View reviewed changes

jihoonson reviewed Mar 12, 2020

View reviewed changes

clintropolis mentioned this pull request Mar 12, 2020

threshold based automatic query prioritization #9493

Merged

8 tasks

clintropolis added 7 commits March 13, 2020 01:42

Merge remote-tracking branch 'upstream/master' into manual-query-laning

13491bb

review stuffs

a05c9fa

doc adjustments

20c10cf

more tests

0bf4232

test adjustment

ee435ad

adjust docs

cdb2d12

Update index.md

d976269

jihoonson approved these changes Mar 14, 2020

View reviewed changes

clintropolis merged commit 69af760 into apache:master Mar 14, 2020

clintropolis deleted the manual-query-laning branch March 14, 2020 03:07

jihoonson added this to the 0.18.0 milestone Mar 26, 2020

Conversation

clintropolis commented Mar 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Uh oh!

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

clintropolis Mar 11, 2020 • 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!

Choose a reason for hiding this comment

Uh oh!

maytasm Mar 11, 2020 • 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!

jihoonson left a comment

Choose a reason for hiding this comment

Uh oh!

clintropolis commented Mar 14, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

clintropolis commented Mar 10, 2020 •

edited

Loading

clintropolis Mar 11, 2020 •

edited

Loading

maytasm Mar 11, 2020 •

edited

Loading