This repository has been archived by the owner on Aug 2, 2022. It is now read-only.
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.
Common changes needed to support dynamic en/disabling of config overrides #294
Common changes needed to support dynamic en/disabling of config overrides #294
Changes from all commits
37be243
570286b
a795e6d
43f194e
b0adabd
6efceb3
14358c4
fdbf2c9
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
callsConfigOverridesHelper.deserialize(lines[1])
in itsprocessEvent
method passing in whatever the writer has written as overrides through theNodeDetailsCollector
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we also want to add a metric here ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.