Overlord to support autoscalers per indexer/middlemanager category by sascha-coenen · Pull Request #9350 · apache/druid

sascha-coenen · 2020-02-11T11:31:36Z

Fixes #8695.

Description

Added support of many autoscalers of different categories to serve tasks of according category.

This PR has:

been self-reviewed.
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.
(https://github.com/apache/incubator-druid/blob/master/licenses.yaml)
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.
been tested in a test Druid cluster.

Key changed/added classes in this PR

CategoriedWorkerBehaviorConfig
CategoriedWorkerProvisioningStrategy
CategoriedWorkerSelectStrategy
PendingTaskBasedWorkerProvisioningStrategy
SimpleWorkerProvisioningStrategy

Initial implementation.

- Changed behavior config structure. - Small refactoring - Extended unit tests

- Extended unit tests

- Extended and refactored unit tests

- Fixed formatting

- Fixed unit tests

- Added "category" field into Autoscaler - Changed config format to support the "category" field - Fixed parameters of unit tests

- Fixed build

- Fixed formatting

- Put category information into Autoscaler - Changed structure of behavior config - Changed CategoriedProvisioningStrategy accordingly - Fixed bug in PendingTaskBasedWorkerProvisioningStrategy - Extended unit tests - Code cleanup and refactoring

- Code refactoring

- Introduced new logic into the existing strategies

- Fixes and refactoring

- Added more test cases - Refactored unit tests

- Fixed code styles

- Used Default Worker Category as a fallback for null worker categories

- Refactored legacy code to avoid NPE - Removed duplicates

- Small fix

- Added more debug info - Removed duplicated code

- Removed debug logging

Overlord can support many autoscalers of different categories apache#8989

sascha-coenen · 2020-02-11T15:59:45Z

Just FYI: we had to move this code contribution from one github repo to another one today.
So we removed the pevious PR that had not been reviewed yet with this one. No code has changed, its just a new PR that originates from a different repo now.
Sorry for the inconvenience.

… Add documentation and javadoc comments. Fixed naming of one class.

… fix spelling in documentation and code to pass spellchecker tests

sascha-coenen · 2020-02-20T10:47:39Z

the spellchecker was failing on a naming issue, so I fixed the spelling in the documentation and also changed wrongly spelled type names in the code accordingly.
Now all checks have passed.

himanshug

partially reviewed, will continue later. just trying to understand things at this point.

himanshug · 2020-02-21T17:43:41Z

+
+import org.joda.time.Period;
+
+public class CategorizedProvisioningConfig extends PendingTaskBasedWorkerProvisioningConfig


note-to-self: Couldn't understand the point of this class as this is overriding all methods and just calling super.xx(..) , maybe I will understand more as I read rest of the PR.

It can be removed. Please ignore.

Thanks Vladi.
Just to update the status: I removed all orhpaned classes.
Changes to the DefaultWorkerBehaviorConfig class are still pending

himanshug · 2020-02-22T05:39:44Z

+  private final List<AutoScaler> autoScalers;
+
+  @JsonCreator
+  public CategorizedWorkerBehaviorConfig(


I think things will be simplified if we removed this class and instead made DefaultWorkerBehaviorConfig have List<AutoScaler> autoScalers with constructor

@JsonCreator public DefaultWorkerBehaviorConfig( @JsonProperty("selectStrategy") WorkerSelectStrategy selectStrategy, @JsonProperty("autoScaler") AutoScaler autoScaler, @JsonProperty("autoScalers") List<AutoScaler> autoScalers ) { this.selectStrategy = selectStrategy; // fail if both "autoscalers" and "autoscaler" fields are non-null this.autoScalers = autoScalers or Collections.singletonList(autoscaler); }

Do you thing it will not confuse users? I'm about having two sections related to autoscalers in the same config.
I'm ok for that. Initially I also had such decision to just extend DefaultWorkerBehaviorConfig. But later I thought that having two sections for autoscalers can be not good for user experience.

Actually, We are keeping autoScaler field only for backward compatibility .. we should really deprecate it and remove its mention from the docs. Eventually it should be removed and we should just have autoScalers .

Ok. No objections. Let's move the autoScalers section to DefaultWorkerBehaviorConfig and remove CategorizedWorkerBehaviorConfig.

I tried to refactor the code based on the above review comments.
The class CategorizedWorkerBehaviorConfig has been removed, its logic been merged into the DefaultworkerBehaviorConfig.
The constructor has been modified according to himanshug's post above.

One thing I noticed recently, is that the new web-console is not treating the behaviour config as a single json object but has separate text boxes for select strategy and autoscaler.

What shall we do about this?

That means, users who want to use autoScalers can't use the console UI to set the spec and would instead have to manually send the http request to coordinator using curl or whatever.
after this PR is merged, an issue should be created for console UI updates to make it possible to add autoScalers .
In this PR, we need to make sure that we stay totally backwards compatible.

got it. thanks.

I am totally down to update the web console to reflect the new functionality.

himanshug · 2020-02-22T05:42:20Z

+ * a worker's category.
+ */
+@JsonTypeName("categorizedTaskBased")
+public class CategorizedWorkerProvisioningStrategy extends AbstractWorkerProvisioningStrategy


Can you explain how this is different from PendingTaskBasedWorkerProvisioningStrategy ?

After reviewing the difference I recalled that it is leftover which can be removed along with CategorizedProvisioningConfig. Originally I separately created the new strategy as an intermediate step to check that I didn't affect the original strategy. So I just forgot to remove it.

yeah, that is what I guessed :)

categorization really cuts across all implementations of ProvisioningStrategy and hence shouldn't be a separate impl on its own.

I tried to refactor the code based on the above review comments. The class CategorizedWorkerProvisioningStrategy has been removed.

Co-Authored-By: Himanshu <g.himanshu@gmail.com>

… remove orphaned classes according to findings duriing code review.

…e CategorizedWorkerBehaviourConfig (apache#9350)

…CategorizedWorkerBehaviourConfig class (apache#9350)

himanshug

reviewed some more, will continue.

himanshug · 2020-03-03T17:38:19Z

+|`selectStrategy`|How to assign tasks to MiddleManagers. Choices are `fillCapacity`, `equalDistribution`, and `javascript`.|equalDistribution|
+|`autoScaler`|Only used if autoscaling is enabled. See below.|null|
+
+##### Categorized Worker Config


this section should be removed and autoScaler be replaced with autoScalers above.

himanshug · 2020-03-03T17:42:06Z

-    this.autoScaler = autoScaler;
+    this.autoScalers = (autoScaler != null) ? Collections.singletonList(autoScaler) : autoScalers;
+    if (this.autoScalers == null) {
+      throw new IllegalArgumentException("Either autoScaler or autoScalers property needs to be provided");


can we remove mention of autoScaler property and deprecate it .. so that it gets removed eventually.

can we add two more checks ?

make sure both autoScaler and autoScalers aren't provided

if autoScalers is provided, make sure each autoScaler element has a different category.

himanshug · 2020-03-03T17:43:58Z

+      return null;
+    }
+    final DefaultWorkerBehaviorConfig workerConfig = (DefaultWorkerBehaviorConfig) workerBehaviorConfig;
+    if (workerConfig.getAutoScalers() == null) {


DefaultWorkerBehaviorConfig constructor shouldn't/doesn't allow instantiation with null autoScalers so this check is redundant.

himanshug · 2020-03-03T17:59:53Z

+    ));
+
+    if (result.size() != autoScalers.size()) {
+      log.warn(


Can we add the check in DefaultWorkerBehaviorConfig to ensure autoscalers with duplicate category aren't provided and fail? This is probably a case of user making an error in configuration.

good point. I changed that.

…ord/setup/CategorizedWorkerSelectStrategy.java Co-Authored-By: Himanshu <g.himanshu@gmail.com>

…ourConfig, doc fixes, clean up in ProvisioningUtil (apache#9350)

himanshug

sorry for the delay, I have gone through whole PR now.

himanshug · 2020-03-04T17:49:03Z

+    if (autoScaler != null && autoScalers != null) {
+      throw new IllegalArgumentException("The autoScaler and autoScalers properties are mutually exclusive");
+    }
+    if (autoScaler == null && autoScalers == null) {
+      throw new IllegalArgumentException("Either autoScaler or autoScalers property must be provided");
+    }


nit: XOR operator is the usual way to handle this type of check.

Suggested change

if (autoScaler != null && autoScalers != null) {

throw new IllegalArgumentException("The autoScaler and autoScalers properties are mutually exclusive");

}

if (autoScaler == null && autoScalers == null) {

throw new IllegalArgumentException("Either autoScaler or autoScalers property must be provided");

}

if (autoScaler == null ^ autoScalers == null) {

throw new IllegalArgumentException("Either(and only one of) autoScaler or autoScalers property must be provided");

}

I made this change but now I get a LGTM alert. It complains that the autoScalers variable can be null in line 64 which would lead to an NPE, but in my opinion LGTM is wrong.
Can you advise?

I agree, lgtm is just wrong in this case.

Also, I see that you are using == instead of ^, while they both produce same result due to interning of boolean values I would prefer ^ which has clearly defined XOR meaning and reader for the code would get immediately context. == is "tricky" way to do what ^ is doing.

himanshug · 2020-03-04T18:15:14Z

+    );
+
+    // select worker according to worker category spec
+    if (workerCategorySpec != null) {


this check is again done in getTaskCategory(..) and is redundant.

himanshug · 2020-03-04T18:16:45Z

  }

  /**
+   *


nit: not sure why this file changed at all :)

;)
right. I reverted the file.

himanshug · 2020-03-11T18:32:49Z

+        List<ImmutableWorkerInfo> categoryWorkers = workersByCategories.getOrDefault(category, Collections.emptyList());
+        currentlyProvisioningMap.putIfAbsent(category, new HashSet<>());
+        Set<String> currentlyProvisioning = this.currentlyProvisioningMap.get(category);
+        currentlyTerminatingMap.putIfAbsent(category, new HashSet<>());
+        Set<String> currentlyTerminating = this.currentlyTerminatingMap.get(category);


nit: replacement similar to other class could be used to avoid instantiation of HashSet here too.

himanshug · 2020-03-11T18:33:41Z

+        currentlyProvisioningMap.putIfAbsent(category, new HashSet<>());
+        Set<String> currentlyProvisioning = this.currentlyProvisioningMap.get(category);
+        currentlyTerminatingMap.putIfAbsent(category, new HashSet<>());
+        Set<String> currentlyTerminating = this.currentlyTerminatingMap.get(category);


nit: same here regarding HashSet instantiation

himanshug · 2020-03-11T18:34:57Z

+        currentlyTerminatingMap.putIfAbsent(category, new HashSet<>());
+        Set<String> currentlyTerminating = this.currentlyTerminatingMap.get(category);
+
+        didProvision = doProvision(


nit: code till here appears to be repeated in many places, if possible, then refactor it into ProvisioningUtil maybe

you are perfectly right about the repeated code logic. However, I'm not able to refactor anything. For one thing, the way that responsibilities are spread across classes seems to make it impossible to refactor just the affected methods but would require a larger rewrite.
On a pragmatic level I'm also facing the issue that executing "mvn test" on just the druid-indexing module takes 15 minutes. If I try to execute the respective testsuite alone (SimpleProvisioningStrategyTest) in IntelliJ, I get an ANTL error:

/opt/repos/druid/core/src/main/java/org/apache/druid/math/expr/ExprListenerImpl.java Error:(28, 40) java: package org.apache.druid.math.expr.antlr does not exist

Can you advise?

Also, one innocent question: I see these "resolve conversation" buttons and wonder whether I am supposed to mark a conversation as resolved if I believe to have addressed a review comment or whether it is rather meant to be the reviewer's privilege to do that.

thanks for checking, I haven't looked that refactoring in details but I believe you.

for the antlr error to go away, do "mvn clean install -DskipTests" in core module whenever you switch branches.

I am supposed to mark a conversation as resolved if I believe to have addressed a review comment or whether it is rather meant to be the reviewer's privilege to do that.

it is a new feature in github and I don't think there is any formal agreement in Druid community over this. But, I would say, PR author should feel free to mark the conversation resolved when they think that they addressed it. If Reviewer doesn't agree then they can mark it "unresolved" and conversation can be further resumed.

Just to randomly chime in, as a reviewer I definitely prefer that the author does not mark my comments as resolved, it makes it harder to come back to the PR and find all of my comments and check to make sure that they are indeed resolved. I think it would be better for whoever left the review comment to resolve it when they are satisfied, but maybe we should reach a consensus as a community on this.

I see, I spent a little more time today to read about the effects of resolving in https://help.github.com/en/github/collaborating-with-issues-and-pull-requests/about-pull-request-reviews#resolving-conversations . Resolving basically folds the conversation so as reviewer, I can see, you might not want that to happen till you verify whether your comment is addressed.
However, In that case reviewers also should have the responsibility of actively marking their comments resolved as and when they are addressed or else PR author is in doubt whether the change has been accepted by reviewer. I haven't really been doing that while reviewing the PRs and that is probably what confused @sascha-coenen .

Co-Authored-By: Himanshu <g.himanshu@gmail.com>

…sioningStrategy (apache#9350) Co-Authored-By: Himanshu <g.himanshu@gmail.com>

lgtm-com · 2020-03-11T23:07:02Z

This pull request introduces 1 alert when merging cd3c76d into 2ef5c17 - view on LGTM.com

new alerts:

1 for Dereferenced variable may be null

lgtm-com · 2020-03-16T22:12:26Z

This pull request introduces 1 alert when merging eb3a001 into 09600db - view on LGTM.com

new alerts:

1 for Dereferenced variable may be null

himanshug · 2020-03-20T23:48:08Z

+1 after #9350 (comment) is addressed. thanks for your patience.

stale · 2020-05-20T06:14:18Z

This pull request has been marked as stale due to 60 days of inactivity. It will be closed in 4 weeks if no further activity occurs. If you think that's incorrect or this pull request should instead be reviewed, please simply write any comment. Even if closed, you can still revive the PR at any time or discuss it on the dev@druid.apache.org list. Thank you for your contributions.

stale · 2020-06-17T07:01:30Z

This pull request/issue has been closed due to lack of activity. If you think that is incorrect, or the pull request requires review, you can revive the PR at any time.

Vladimir Iordanov and others added 24 commits December 4, 2019 15:37

Support different types of MiddleManagers by Autoscaler apache#8695

71bf232

Initial implementation.

Support different types of MiddleManagers by Autoscaler apache#8695

22f7529

- Changed behavior config structure. - Small refactoring - Extended unit tests

Support different types of MiddleManagers by Autoscaler apache#8695

b61fee9

- Extended unit tests

Support different types of MiddleManagers by Autoscaler apache#8695

66d3ebe

- Extended and refactored unit tests

Support different types of MiddleManagers by Autoscaler apache#8695

ba88a3c

- Extended and refactored unit tests

Support different types of MiddleManagers by Autoscaler apache#8695

d929a4e

- Extended and refactored unit tests

Support different types of MiddleManagers by Autoscaler apache#8695

66f63cc

- Fixed formatting

Support different types of MiddleManagers by Autoscaler apache#8695

320f0c2

- Fixed unit tests

Support different types of MiddleManagers by Autoscaler apache#8695

9fba4d2

- Added "category" field into Autoscaler - Changed config format to support the "category" field - Fixed parameters of unit tests

Support different types of MiddleManagers by Autoscaler apache#8695

5b2e0c6

- Fixed build

Support different types of MiddleManagers by Autoscaler apache#8695

66fbec5

- Fixed formatting

Support different types of MiddleManagers by Autoscaler apache#8695

e12c2a6

- Code refactoring

Support different types of MiddleManagers by Autoscaler apache#8695

8bd1959

- Code refactoring

Support different types of MiddleManagers by Autoscaler apache#8695

d530f03

- Introduced new logic into the existing strategies

Support different types of MiddleManagers by Autoscaler apache#8695

16deb7b

- Fixes and refactoring

Support different types of MiddleManagers by Autoscaler apache#8695

c5f1f93

- Added more test cases - Refactored unit tests

Support different types of MiddleManagers by Autoscaler apache#8695

3028f32

- Fixed code styles

Support different types of MiddleManagers by Autoscaler apache#8695

7c0a379

- Used Default Worker Category as a fallback for null worker categories

Support different types of MiddleManagers by Autoscaler apache#8695

10f51a7

- Refactored legacy code to avoid NPE - Removed duplicates

Support different types of MiddleManagers by Autoscaler apache#8695

931ebba

- Small fix

Support different types of MiddleManagers by Autoscaler apache#8695

03f5e62

- Added more debug info - Removed duplicated code

Support different types of MiddleManagers by Autoscaler apache#8695

e16e40b

- Removed debug logging

Merge pull request #1 from VladimirIordanov/feature-8695

359f73f

Overlord can support many autoscalers of different categories apache#8989

sascha-coenen requested review from QiuMM and gianm February 11, 2020 15:57

Support different types of MiddleManagers by Autoscaler apache#8695 -…

56b2e60

… Add documentation and javadoc comments. Fixed naming of one class.

himanshug mentioned this pull request Feb 14, 2020

Overlord can support many autoscalers of different categories #8989

Closed

8 tasks

sascha-coenen changed the title ~~Overlord can support many autoscalers of different categories~~ Overlord to support autoscalers per indexer/middlemanager category Feb 19, 2020

Support different types of MiddleManagers by Autoscaler apache#8695 -…

57065ff

… fix spelling in documentation and code to pass spellchecker tests

himanshug reviewed Feb 22, 2020

View reviewed changes

sascha-coenen and others added 5 commits February 26, 2020 21:12

Update docs/configuration/index.md

7ca56ca

Co-Authored-By: Himanshu <g.himanshu@gmail.com>

Support different types of MiddleManagers by Autoscaler apache#8695 -…

d9a0991

… remove orphaned classes according to findings duriing code review.

Refactor DEFAULT_AUTOSCALER_CATEGORY constant in preparation to remov…

09449e1

…e CategorizedWorkerBehaviourConfig (apache#9350)

Move autoScalers property to DefaultWorkerBehaviourConfig and remove …

70e26f3

…CategorizedWorkerBehaviourConfig class (apache#9350)

Fixed compile error in WorkerBehaviorConfigTest (apache#9350)

64a0836

himanshug reviewed Mar 3, 2020

View reviewed changes

sascha-coenen and others added 2 commits March 4, 2020 00:22

Update indexing-service/src/main/java/org/apache/druid/indexing/overl…

9a1f7c7

…ord/setup/CategorizedWorkerSelectStrategy.java Co-Authored-By: Himanshu <g.himanshu@gmail.com>

Addressing review comments - add sanity checks in DefaultWorkerBehavi…

adb73cc

…ourConfig, doc fixes, clean up in ProvisioningUtil (apache#9350)

himanshug reviewed Mar 11, 2020

View reviewed changes

sascha-coenen and others added 4 commits March 11, 2020 22:13

refactor sanity checks in class DefaultWorkerBehaviour (apache#9350)

c8dfcbd

Co-Authored-By: Himanshu <g.himanshu@gmail.com>

Avoid unnecessary instantiations in class PendingTaskBasedWorkerProvi…

8ac202c

…sioningStrategy (apache#9350) Co-Authored-By: Himanshu <g.himanshu@gmail.com>

Avoid unnecessary instantiations in class PendingTaskBasedWorkerProvi…

bf79d0b

…sioningStrategy (apache#9350) Co-Authored-By: Himanshu <g.himanshu@gmail.com>

Avoid unnecessary instantiations in class PendingTaskBasedWorkerProvi…

cd3c76d

…sioningStrategy (apache#9350) Co-Authored-By: Himanshu <g.himanshu@gmail.com>

Addressing review comments (apache#9350)

eb3a001

stale Bot added the stale label May 20, 2020

stale Bot closed this Jun 17, 2020


		import org.joda.time.Period;

		public class CategorizedProvisioningConfig extends PendingTaskBasedWorkerProvisioningConfig

                 }
                 /**
+                 *

Conversation

sascha-coenen commented Feb 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Key changed/added classes in this PR

Uh oh!

sascha-coenen commented Feb 11, 2020

Uh oh!

sascha-coenen commented Feb 20, 2020

Uh oh!

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

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 Mar 3, 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!

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

Uh oh!

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

sascha-coenen commented Feb 11, 2020 •

edited

Loading

himanshug left a comment •

edited

Loading

himanshug Mar 3, 2020 •

edited

Loading