Skip to content
This repository has been archived by the owner on Aug 2, 2022. It is now read-only.

Add ITs for JVM old gen policy #499

Merged
merged 5 commits into from
Oct 29, 2020
Merged

Add ITs for JVM old gen policy #499

merged 5 commits into from
Oct 29, 2020

Conversation

rguo-aws
Copy link
Contributor

…Rest endpoint

Fixes #:

Description of changes:
Add ITs for JVM old gen policy. This PR fixes a few minor issues in IT framework to allow IT to retrieve persisted action list from rest endpoint. We also add six ITs to cover the happy scenario of old gen policy in JVM decider.
Those ITs are :
ITs that triggers LevelOne/LevelTwo/LevelThree action builder (dedicated master / multinode)

Tests:

If new tests are added, how long do the new ones take to complete

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

yojs
yojs previously approved these changes Oct 28, 2020
@@ -278,7 +278,7 @@ synchronized int insertRow(String tableName, List<Object> row) throws SQLExcepti
recordsWithMaxFieldValue = create.select().from(tableName).where(DSL.field(field)
.eq(create.select(max(field)).from(tableName))).fetch();
} catch (DataAccessException dex) {
LOG.error("Error querying table {}", tableName, dex);
//LOG.warn("Error querying table {}", tableName, dex);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we just remove it ?

@@ -120,4 +118,39 @@ public CacheClearAction build() {
return new CacheClearAction(appContext, coolOffPeriodInMillis, canUpdate);
}
}

public static class Summary {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we make this as an util class or rename as CacheClearActionSummary?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this Summary is only used as a Json container within each action class(similar to the Builder class within the action class), I would like to leave it as it is. We can make them as separate classes if needed in the future

Comment on lines 173 to 175
@AErrorPatternIgnored(
pattern = "PersistableSlidingWindow:<init>()",
reason = "Persistence base path can be null for integration test.")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is fixed with Sid's PR. We dont need to add them here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for catching this. Yes. let me remove those in my next PRs(I am creating a few more ITs to cover corner cases in JVM decider)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done, removed this pattern

@@ -72,7 +73,7 @@ protected double getMaxOldGenSizeOrDefault(final double defaultValue) {
}
double ret =
SQLParsingUtil
.readDataFromSqlResult(heapMaxMetric.getData(), MEM_TYPE.getField(), OLD_GEN.toString(), MetricsDB.MAX);
.readDataFromSqlResult(heapMaxMetric.getData(), MEM_TYPE.getField(), HEAP.toString(), MetricsDB.MAX);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see that you have updated OLD_GEN to HEAP. The method says getMaxOldGenSizeOrDefault. Is there a reason why this change was made? Do we want to rename the method?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, this was a bug I found in old gen RCA. Heap usage is supposed to be calculated by (old gen usage)/(max total heap size). but for some reason, we calculated it by (old gen usage)/(max old gen size)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done. added a new function to retrieve HEAP and leave the OLD_GEN unchanged

sruti1312
sruti1312 previously approved these changes Oct 29, 2020
@sruti1312 sruti1312 requested review from sruti1312 and yojs October 29, 2020 01:10
@sruti1312
Copy link
Contributor

sruti1312 commented Oct 29, 2020

com.amazon.opendistro.elasticsearch.performanceanalyzer.store.rca.hotheap.HighHeapUsageOldGenRcaTest

  Test testHighHeapOldGenRca FAILED

  java.lang.AssertionError

There are some tests failing

@rguo-aws rguo-aws dismissed stale reviews from sruti1312 and yojs via 2ff3041 October 29, 2020 01:39
@rguo-aws rguo-aws merged commit f07671c into master Oct 29, 2020
@rguo-aws rguo-aws deleted the rguo-jvm-decider-IT2 branch October 29, 2020 17:29
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants