This repository has been archived by the owner on Aug 2, 2022. It is now read-only.
Make RCA framework NOT use ClusterDetailsEventProcessor #274
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov Report
@@ Coverage Diff @@
## master #274 +/- ##
============================================
+ Coverage 66.14% 66.21% +0.07%
- Complexity 1789 1802 +13
============================================
Files 270 272 +2
Lines 11988 12027 +39
Branches 952 953 +1
============================================
+ Hits 7929 7964 +35
- Misses 3744 3750 +6
+ Partials 315 313 -2 Continue to review full report at Codecov.
|
rguo-aws
reviewed
Jul 15, 2020
| } | ||
|
|
||
| public InstanceDetails getInstanceDetails() { | ||
| return this.appContext.getMyInstanceDetails(); |
There was a problem hiding this comment.
do wen need to check whether appContext is null here to avoid null pointer exception ?
There was a problem hiding this comment.
Thanks a lot @rguo-aws Will add it
rguo-aws
reviewed
Jul 15, 2020
| @@ -187,4 +194,20 @@ public void setLocalFlowUnit(T localFlowUnit) { | |||
| public void readRcaConf(RcaConf conf) { | |||
| return; | |||
| } | |||
|
|
|||
| public void setAppContext(final AppContext appContext) { | |||
There was a problem hiding this comment.
add javadoc for this function ?
There was a problem hiding this comment.
Do we really want a javadoc for a setter ?
Using the static classes have problems as one instance of JVM can have only one instance of the class running. ClusterDetailsEventProcessor is an important class as it contains the instance details. If we want to simutate multiple RCAControllers running as part of a single JVM for the purpose of IntegrationTest framework, we should make ClusterDetailsEventProcessor a non-static class, such that each RcaController can work with its own. This is not the complete removal of the static methods in the ClusterDetailsEventProcessor, but just making the RCA framework not use them. There will be follow up PRs that will eventually make ClusterDetailsEventProcessor non-static
…deDetails" except for periodic samplers
…or from the RCA code. Except for the Periodic Samplers.
…icsRequestHandler
yojs
force-pushed
the
refactor-ClusterDetailsEventProcessor
branch
from
b90ee07
to
fc0f4ef
Compare
ktkrg
approved these changes
Jul 15, 2020
src/main/java/com/amazon/opendistro/elasticsearch/performanceanalyzer/AppContext.java
Outdated
Show resolved
Hide resolved
ktkrg
reviewed
Jul 15, 2020
|
Good point ! removed all unused imports |
rguo-aws
approved these changes
Jul 16, 2020
sidheart
pushed a commit
that referenced
this pull request
Jul 24, 2020
This reverts commit 95c19b6.
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Using the static classes have problems as one instance of JVM can have only one instance of the class running. ClusterDetailsEventProcessor is an important class as it contains the instance details. If we want to simutate multiple RCAControllers running as part of a single JVM for the purpose of IntegrationTest framework, we should make ClusterDetailsEventProcessor a non-static class, such that each RcaController can work with its own. This is not the complete removal of the static methods in the ClusterDetailsEventProcessor, but just making the RCA framework not use them. There will be follow up PRs that will eventually make ClusterDetailsEventProcessor non-static
Issue #, if available:
Description of changes:
Tests:
Code coverage percentage for this patch:
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.