JVM decider #326

rguo-aws · 2020-07-31T18:12:23Z

Issue #:

Description of changes:
Add JVM decider to RCA framework

Tests:
WIP

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

codecov · 2020-07-31T18:18:31Z

Codecov Report

Merging #326 into master will decrease coverage by 0.96%.
The diff coverage is 16.85%.

@@             Coverage Diff              @@
##             master     #326      +/-   ##
============================================
- Coverage     70.54%   69.57%   -0.97%     
- Complexity     2310     2371      +61     
============================================
  Files           307      318      +11     
  Lines         13686    14273     +587     
  Branches       1142     1199      +57     
============================================
+ Hits           9655     9931     +276     
- Misses         3669     3976     +307     
- Partials        362      366       +4

Impacted Files	Coverage Δ	Complexity Δ
...rmanceanalyzer/decisionmaker/deciders/Decider.java	`68.00% <ø> (+2.48%)`	`5.00 <0.00> (ø)`
.../decisionmaker/deciders/jvm/HeapHealthDecider.java	`0.00% <0.00%> (ø)`	`0.00 <0.00> (?)`
...er/deciders/jvm/old_gen/LevelOneActionBuilder.java	`0.00% <0.00%> (ø)`	`0.00 <0.00> (?)`
.../deciders/jvm/old_gen/LevelThreeActionBuilder.java	`0.00% <0.00%> (ø)`	`0.00 <0.00> (?)`
...er/deciders/jvm/old_gen/LevelTwoActionBuilder.java	`0.00% <0.00%> (ø)`	`0.00 <0.00> (?)`
...ker/deciders/jvm/old_gen/OldGenDecisionPolicy.java	`0.00% <0.00%> (ø)`	`0.00 <0.00> (?)`
...erformanceanalyzer/rca/framework/core/RcaConf.java	`51.96% <33.33%> (-0.26%)`	`32.00 <1.00> (-1.00)`
...nfigs/decider/jvm/LevelTwoActionBuilderConfig.java	`42.85% <42.85%> (ø)`	`1.00 <1.00> (?)`
...onfigs/decider/jvm/OldGenDecisionPolicyConfig.java	`45.00% <45.00%> (ø)`	`1.00 <1.00> (?)`
...nfigs/decider/jvm/LevelOneActionBuilderConfig.java	`50.00% <50.00%> (ø)`	`1.00 <1.00> (?)`
... and 28 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2f6ba39...d7ef339. Read the comment docs.

yojs

My two cents would be that we simplify the LevelAction classes further. I understand that once we have decided the level of jvm old-gen pressure we are in, the classes help us decide what we want to touch and in what order and how much of the knob we want to turn for it (decrease it in steps or wipe it clean).

It might make sense to define this in a yml and have just one level class that reads and takes actions. We may decide if the yml will be for code level config or something that operators can change but having it might make the logic more clear. What do you think ?

...istro/elasticsearch/performanceanalyzer/decisionmaker/actions/ModifyCacheCapacityAction.java

...on/opendistro/elasticsearch/performanceanalyzer/rca/store/collector/NodeConfigCacheUtil.java

...sticsearch/performanceanalyzer/decisionmaker/deciders/jvm/old_gen/LevelOneActionBuilder.java

...sticsearch/performanceanalyzer/decisionmaker/deciders/jvm/old_gen/LevelTwoActionBuilder.java

...distro/elasticsearch/performanceanalyzer/decisionmaker/actions/ModifyCacheMaxSizeAction.java

...istro/elasticsearch/performanceanalyzer/decisionmaker/actions/ModifyQueueCapacityAction.java

.../elasticsearch/performanceanalyzer/decisionmaker/deciders/jvm/old_gen/BaseActionBuilder.java

...sticsearch/performanceanalyzer/decisionmaker/deciders/jvm/old_gen/LevelOneActionBuilder.java

...icsearch/performanceanalyzer/decisionmaker/deciders/jvm/old_gen/LevelThreeActionBuilder.java

sidheart · 2020-08-05T23:54:15Z

...asticsearch/performanceanalyzer/decisionmaker/deciders/jvm/old_gen/OldGenDecisionPolicy.java

+      return LevelThreeActionBuilder.newBuilder(esNode, nodeConfigCache).build();
+    }
+    else if (oldGenUsage >= OLD_GEN_THRESHOLD_LEVEL_TWO) {
+      return LevelTwoActionBuilder.newBuilder(esNode, nodeConfigCache).build();
+    }
+    else if (oldGenUsage >= OLD_GEN_THRESHOLD_LEVEL_ONE) {
+      return LevelOneActionBuilder.newBuilder(esNode, nodeConfigCache).build();


This is very neat for this particular use case, but these actionbuilders aren't reusable

After rereading this my thoughts are the same. I don't understand what problem we were trying to solve by using the builder pattern here. We don't actually use the builders as legitimate builders, they're just wrappers around specific logic for generating a list of actions given a known bucket level. Why can't the static variables be held in this class with the List generated in actions() itself? Who would reuse these classes down the line?

The only moderately complex logic seems to live entirely in LevelTwoActionBuilder#actionPriorityForQueue(), but might not this just live in a function in this class?

You are right, the complex logic right now only sit in Level two but as I mention above, we need to collect more data from experiment and then make changes based on that. So now level 1 & 3 are more like placeholder with very simple logics in it and each bucket in JVM decider can be evolved independently in the future.

I agree with Sid, I don't really see a need for the common base class here. We have different strategies for each level that we want to be able to evolve independently. A simple interface might be enough.

This base class approach was suggested by Joydeep because he though we can move those common code into base class. Yes, I agree with you. We don't necessarily need inheritance here and each level can evolve independently. Removed the base class

...distro/elasticsearch/performanceanalyzer/decisionmaker/actions/ModifyCacheMaxSizeAction.java

...istro/elasticsearch/performanceanalyzer/decisionmaker/actions/ModifyQueueCapacityAction.java

.../elasticsearch/performanceanalyzer/decisionmaker/deciders/jvm/old_gen/BaseActionBuilder.java

...sticsearch/performanceanalyzer/decisionmaker/deciders/jvm/old_gen/LevelOneActionBuilder.java

...sticsearch/performanceanalyzer/decisionmaker/deciders/jvm/old_gen/LevelTwoActionBuilder.java

vigyasharma · 2020-08-19T17:39:16Z

...asticsearch/performanceanalyzer/decisionmaker/deciders/jvm/old_gen/OldGenDecisionPolicy.java

+      return LevelThreeActionBuilder.newBuilder(esNode, nodeConfigCache).build();
+    }
+    else if (oldGenUsage >= OLD_GEN_THRESHOLD_LEVEL_TWO) {
+      return LevelTwoActionBuilder.newBuilder(esNode, nodeConfigCache).build();
+    }
+    else if (oldGenUsage >= OLD_GEN_THRESHOLD_LEVEL_ONE) {
+      return LevelOneActionBuilder.newBuilder(esNode, nodeConfigCache).build();


I agree with Sid, I don't really see a need for the common base class here. We have different strategies for each level that we want to be able to evolve independently. A simple interface might be enough.

...asticsearch/performanceanalyzer/decisionmaker/deciders/jvm/old_gen/OldGenDecisionPolicy.java

vigyasharma

There's some conflict across PRs. Will wait till we consolidate before approving.

...distro/elasticsearch/performanceanalyzer/decisionmaker/actions/ModifyCacheMaxSizeAction.java

.../opendistro/elasticsearch/performanceanalyzer/decisionmaker/deciders/CacheHealthDecider.java

...sticsearch/performanceanalyzer/decisionmaker/deciders/jvm/old_gen/LevelOneActionBuilder.java

vigyasharma · 2020-08-31T17:21:52Z

...sticsearch/performanceanalyzer/decisionmaker/deciders/jvm/old_gen/LevelOneActionBuilder.java

+  // TODO : read priority from yml if customer wants to override default ordering
+  @Override
+  protected void actionPriorityFilter() {
+    actionFilter.put(ResourceEnum.FIELD_DATA_CACHE, true);


I see. But from current changes, I don't see any action being added to actionFilter as false, so it seems like we just add actions to the actionFilter map as true, and pick all of them in build for each level*ActionBuilder. What am i missing?

...mazon/opendistro/elasticsearch/performanceanalyzer/rca/configs/decider/CacheBoundConfig.java

...sticsearch/performanceanalyzer/decisionmaker/deciders/jvm/old_gen/LevelTwoActionBuilder.java

...o/elasticsearch/performanceanalyzer/rca/configs/decider/jvm/LevelOneActionBuilderConfig.java

...o/elasticsearch/performanceanalyzer/rca/configs/decider/jvm/LevelTwoActionBuilderConfig.java

vigyasharma

Looks good. Will you add tests in a separate PR? If so, can you create an issue to track it?

vigyasharma · 2020-09-03T06:03:13Z

...istro/elasticsearch/performanceanalyzer/decisionmaker/actions/configs/CacheActionConfig.java

+  public double getStepSize(ResourceEnum cacheType) {
+    ThresholdConfig<Double> threshold = getThresholdConfig(cacheType);
+    return (threshold.upperBound() - threshold.lowerBound()) / (double) getTotalStepCount();
+  }


...sticsearch/performanceanalyzer/decisionmaker/deciders/jvm/old_gen/LevelTwoActionBuilder.java

vigyasharma · 2020-09-03T06:12:24Z

...sticsearch/performanceanalyzer/decisionmaker/deciders/jvm/old_gen/LevelTwoActionBuilder.java

+      // tie breaker
+      else {
+        if (workLoadTypeConfig.preferIngestOverSearch()) {
+          actionFilter.put(ResourceEnum.WRITE_THREADPOOL, true);


if ingest is preferable, we should downsize search queue, no?

thanks for catching this. Yes we should downsize search queue instead.

vigyasharma · 2020-09-03T06:14:21Z

...endistro/elasticsearch/performanceanalyzer/rca/configs/decider/CachePriorityOrderConfig.java

+    priorityOrder = new Config(PRIORITY_ORDER_CONFIG_NAME, configs.getValue(),
+        DEFAULT_PRIORITY_ORDER, List.class);


Does this work? I was having trouble casting with generics

yes, casting to a raw list still works. it can still pass the UTs that Adithya wrote which overrides the config settings in rca.conf and read this object to verify the changes.

vigyasharma · 2020-09-03T06:17:29Z

...zon/opendistro/elasticsearch/performanceanalyzer/rca/configs/decider/WorkLoadTypeConfig.java

+  private Predicate<List<String>> listValidator;
+
+  public WorkLoadTypeConfig(NestedConfig configs) {
+    listValidator = (list) -> {


Since there is only search and ingest (2 values), we can make this simpler by renaming the config. Instead of taking a list, we can do things like:

{ "decider-config-settings": { "prefer-ingest-over-search": true } }

done. changed the config to
"workload-type": {
"prefer-ingest": true,
"prefer-search": false
},

This will allow user to not assign workload preference to any queue type

sidheart

lgtm thanks for cleaning this up

sidheart · 2020-09-03T20:13:37Z

...istro/elasticsearch/performanceanalyzer/decisionmaker/actions/configs/CacheActionConfig.java

+  public double getStepSize(ResourceEnum cacheType) {
+    ThresholdConfig<Double> threshold = getThresholdConfig(cacheType);
+    return (threshold.upperBound() - threshold.lowerBound()) / (double) getTotalStepCount();
+  }


JVM decider framework

5274a13

rguo-aws added the feature New feature label Jul 31, 2020

rguo-aws requested review from vigyasharma, sruti1312 and khushbr July 31, 2020 18:12

rguo-aws requested a review from yojs August 4, 2020 17:25

yojs reviewed Aug 5, 2020

View reviewed changes

rguo-aws added 2 commits August 5, 2020 10:47

Merge branch 'master' into rguo-jvm-decider

c11c54b

Address PR comments

3ffecf2

rguo-aws requested review from sidheart and yojs August 5, 2020 21:03

sidheart suggested changes Aug 6, 2020

View reviewed changes

Add UTs, WIP

a8cea38

vigyasharma suggested changes Aug 19, 2020

View reviewed changes

rguo-aws added 3 commits August 19, 2020 13:37

Merge remote-tracking branch 'origin' into rguo-jvm-decider

848c6a7

Address PR comments

ead5c04

clean up code

158721b

vigyasharma suggested changes Aug 31, 2020

View reviewed changes

vigyasharma reviewed Aug 31, 2020

View reviewed changes

...sticsearch/performanceanalyzer/decisionmaker/deciders/jvm/old_gen/LevelTwoActionBuilder.java Outdated Show resolved Hide resolved

Merge remote-tracking branch 'origin' into rguo-jvm-decider

64d3f72

vigyasharma reviewed Sep 1, 2020

View reviewed changes

...o/elasticsearch/performanceanalyzer/rca/configs/decider/jvm/LevelOneActionBuilderConfig.java Outdated Show resolved Hide resolved

vigyasharma reviewed Sep 1, 2020

View reviewed changes

...o/elasticsearch/performanceanalyzer/rca/configs/decider/jvm/LevelTwoActionBuilderConfig.java Outdated Show resolved Hide resolved

vigyasharma reviewed Sep 1, 2020

View reviewed changes

...o/elasticsearch/performanceanalyzer/rca/configs/decider/jvm/LevelTwoActionBuilderConfig.java Show resolved Hide resolved

rguo-aws added 2 commits September 1, 2020 18:16

rebase to the latest master

e9c6ead

Address more PR comments

38366cd

rguo-aws requested review from vigyasharma and sidheart September 3, 2020 02:31

vigyasharma reviewed Sep 3, 2020

View reviewed changes

Address more comments

d7ef339

rguo-aws requested a review from vigyasharma September 3, 2020 19:34

sidheart approved these changes Sep 3, 2020

View reviewed changes

vigyasharma approved these changes Sep 3, 2020

View reviewed changes

rguo-aws merged commit 1decacb into master Sep 3, 2020

rguo-aws deleted the rguo-jvm-decider branch September 3, 2020 21:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

JVM decider #326

JVM decider #326

rguo-aws commented Jul 31, 2020

codecov bot commented Jul 31, 2020 •

edited

Loading

yojs left a comment

sidheart Aug 5, 2020

sidheart Aug 6, 2020

sidheart Aug 6, 2020

rguo-aws Aug 11, 2020

vigyasharma Aug 19, 2020

rguo-aws Aug 20, 2020

vigyasharma Aug 19, 2020

vigyasharma left a comment

vigyasharma Aug 31, 2020

vigyasharma left a comment

vigyasharma Sep 3, 2020

sidheart Sep 3, 2020

vigyasharma Sep 3, 2020

rguo-aws Sep 3, 2020

vigyasharma Sep 3, 2020

rguo-aws Sep 3, 2020

vigyasharma Sep 3, 2020

rguo-aws Sep 3, 2020

sidheart left a comment

sidheart Sep 3, 2020

		priorityOrder = new Config(PRIORITY_ORDER_CONFIG_NAME, configs.getValue(),
		DEFAULT_PRIORITY_ORDER, List.class);

JVM decider #326

JVM decider #326

Conversation

rguo-aws commented Jul 31, 2020

codecov bot commented Jul 31, 2020 • edited Loading

Codecov Report

yojs left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vigyasharma left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vigyasharma left a 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

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

sidheart left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jul 31, 2020 •

edited

Loading