Common changes needed to support dynamic en/disabling of config overrides #294
Conversation
Codecov Report
@@ Coverage Diff @@
## master #294 +/- ##
============================================
+ Coverage 67.26% 67.37% +0.10%
- Complexity 1949 1987 +38
============================================
Files 286 291 +5
Lines 12712 12817 +105
Branches 1034 1042 +8
============================================
+ Hits 8551 8635 +84
- Misses 3798 3816 +18
- Partials 363 366 +3
Continue to review full report at Codecov.
|
...om/amazon/opendistro/elasticsearch/performanceanalyzer/config/overrides/ConfigOverrides.java
Show resolved
Hide resolved
...om/amazon/opendistro/elasticsearch/performanceanalyzer/config/overrides/ConfigOverrides.java
Show resolved
Hide resolved
| }); | ||
|
|
||
| if (configOverrides == null && exception[0] != null) { | ||
| // re throw the exception that was consumed while deserializing. |
There was a problem hiding this comment.
Do we also want to add a metric here ?
There was a problem hiding this comment.
I've added the metric where we call deserialize() in ClusterDetailsEventProcessor, I'll send the PR after this gets merged as that PR depends on this change.
| * @throws IOException if conversion runs into an IOException. | ||
| */ | ||
| public static ConfigOverrides deserialize(final String overrides) throws IOException { | ||
| final IOException[] exception = new IOException[1]; |
There was a problem hiding this comment.
Can you explain if this is a one element array, why does it needs to be an array, can't it just be a single reference ?
There was a problem hiding this comment.
We can't use a local variable inside the lambda because it needs to be effectively final so we need a one element array(or any wrapper around the exception) to fish the exception out of the lambda into the calling code. I've added this as a javadoc comment as well.
| */ | ||
| public class ConfigOverridesHelper { | ||
|
|
||
| private static final ObjectMapper MAPPER = new ObjectMapper(); |
There was a problem hiding this comment.
This is a static final object. Some of the decider actions are node level. I don't think this will work with the RCA-IT framework if we just have a static object mapper here, unless the Overrides object haves nodeId as a parameter, in my opinion.
There was a problem hiding this comment.
This is only to help deserialize the string from ClusterDetailsEventProcessor into the override object. It's then updated in the RcaConf. This change will come in the next PR. This PR is only introducing the common classes needed by both the reader and the writer.
Edit:
It's used only as a helper, and it didn't make sense to create an instance of the helper object and pass it around to serialize and deserialize the overrides so decided to make it static. It does not mutate any state and works only on the arguments supplied.
The ClusterDetailsEventProcessor calls ConfigOverridesHelper.deserialize(lines[1]) in its processEvent method passing in whatever the writer has written as overrides through the NodeDetailsCollector.
Issue #:
Fixes #290
Description of changes:
This change adds the common code needed for supporting dynamic enabling and disabling of rcas, deciders, and actions.
The actual changes for applying the config overrides will be in the next PR after this gets merged. It's currently in a branch: performance-analyzer-rca/ktkrg-ed2 branch.
Tests:
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.