Add NodeConfigCollector to collect node configs(threadpool capacity etc.) from ES #252
Conversation
Codecov Report
@@ Coverage Diff @@
## master #252 +/- ##
============================================
- Coverage 66.65% 66.49% -0.16%
- Complexity 1838 1853 +15
============================================
Files 272 275 +3
Lines 12129 12229 +100
Branches 967 976 +9
============================================
+ Hits 8084 8132 +48
- Misses 3723 3770 +47
- Partials 322 327 +5
Continue to review full report at Codecov.
|
...distro/elasticsearch/performanceanalyzer/rca/store/rca/remediation/NodeConfigurationRca.java
Outdated
Show resolved
Hide resolved
...distro/elasticsearch/performanceanalyzer/rca/store/rca/remediation/NodeConfigurationRca.java
Outdated
Show resolved
Hide resolved
| private int writeQueueCapacity; | ||
| private int searchQueueCapacity; | ||
|
|
||
| public <M extends Metric> NodeConfigurationRca(int rcaPeriod, M threadPool_queueCapacity) { |
There was a problem hiding this comment.
Can you briefly describe the rationale of using an RCA node for this ? Do you think it would be cleaner if we add a new node type called ESConfigNode ?
There was a problem hiding this comment.
@yojs are you suggesting that this should be different kind a NonLeafNode and not extend from RCA. With a FlowUnit that is more tuned to node configurations?
I think that is cleaner in general. Will need changes to PersistorBase logic which needs to be refactored anyway to work with non flow units that are not from RCAs. It can be picked separately.
There was a problem hiding this comment.
Created a new type of node EsConfigNode and extend the class as PerformanceControllerConfigCollector
rguo-aws
changed the title
| /** | ||
| * this is a base class for node(vertex) in RCA graph that reads configuration settings from ES. | ||
| */ | ||
| public abstract class EsConfigNode extends NonLeafNode<ResourceFlowUnit<HotNodeSummary>> { |
There was a problem hiding this comment.
I was actually thinking of having a custom flow unit for this type of node. It doesn't really fit into the nested HotNodeSummary and HotResourceSummary lists - the terms healthy/unhealthy do not apply to a given queue capacity conf. The new flow unit could be simply have resource and values for those resources (for each node).
There was a problem hiding this comment.
Agree. that is actually what I am planing to do. but that requires refactoring in base cluster RCA. (i.e. need to create a base node level summary and extend it to something like EsConfigFlowunit). At this moment, we still have to add this to the HotNodeSummary so that it can be parsed by the cluster level RCA. I will create an issue and refactor it altogether.
There was a problem hiding this comment.
Add NodeConfigFlowUnit as a new type of Flowunit to carry node config settings across RCA graph
rguo-aws
added
duplicate
rguo-aws
changed the title
| /** | ||
| * a flowunit type to carry ES node configurations (queue/cache capacities, etc.) | ||
| */ | ||
| public class NodeConfigFlowUnit extends ResourceFlowUnit<HotNodeSummary> { |
There was a problem hiding this comment.
Shouldn't the NodeConfig use its own summary such as NodeConfigSummary ?
There was a problem hiding this comment.
the problem of creating its own summary is this summary needs to be sent over to gRPC so we have to create a separate protobuf message for that. It would be better to keep the protobuf message as is to avoid duplicated code and create a new flowunit type as a wrapper to read/write config settings
Issue #, if available:
#293
Description of changes:
Add a base class EsConfigNode as node level config collector
Create derived class NodeConfigCollector to collect threadpool config settings from each node and wrap it as flowunit to send to cluster RCA node.
Create unit test for this collector
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.