Add a cluster level collector for node config settings #298
Conversation
… config cache in AppContext
Codecov Report
@@ Coverage Diff @@
## master #298 +/- ##
============================================
- Coverage 66.71% 66.58% -0.14%
- Complexity 1869 1896 +27
============================================
Files 276 284 +8
Lines 12307 12559 +252
Branches 982 1019 +37
============================================
+ Hits 8211 8362 +151
- Misses 3764 3848 +84
- Partials 332 349 +17 Continue to review full report at Codecov.
|
| if (ret == null) { | ||
| return Double.NaN; | ||
| } |
There was a problem hiding this comment.
Since this is not going to sql db, I don't think we need NaN value and check. Let's instead throw a checked exception here for consumer code safety? (anyway the check has to be handled by the caller; exception will help protect against cases where callers forget the NaN check).
There was a problem hiding this comment.
done, throw an exception if config does not exist in cache
...amazon/opendistro/elasticsearch/performanceanalyzer/rca/store/collector/NodeConfigCache.java
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| public void put(NodeKey nodeKey, Resource config, double value) { | ||
| nodeConfigCache.put(new NodeConfigKey(nodeKey, config), value); |
There was a problem hiding this comment.
Do we want to check here for Non-Null, Non-Negative values before putting them in HashMap?
There was a problem hiding this comment.
we perform this check in the node level NodeConfigCollector when collection configs from DB.
...amazon/opendistro/elasticsearch/performanceanalyzer/rca/store/collector/NodeConfigCache.java
Show resolved
Hide resolved
| private final NodeConfigCollector nodeConfigCollector; | ||
|
|
||
| public NodeConfigClusterCollector(final NodeConfigCollector nodeConfigCollector) { | ||
| super(0, 5); |
There was a problem hiding this comment.
The constants here make the code unreadable, if we can add a variable with name suggesting what the constant represents, it is helpful.
Additionally, later we should refactor to introduce a defaultIntervalPeriod and use is across all collector and similarly for RCAs as well.
There was a problem hiding this comment.
yes, we can create a static const here. But since we will do another refactoring to remove the 5 second intervals form the super() constructor completely, it would be better to leave it as is at this moment ?
There was a problem hiding this comment.
Makes sense, please add an issue for this.
...distro/elasticsearch/performanceanalyzer/rca/store/collector/NodeConfigClusterCollector.java
Show resolved
Hide resolved
| //unbounded cache with eviction timeout set to 10 mins | ||
| public NodeConfigCache() { | ||
| nodeConfigCache = | ||
| CacheBuilder.newBuilder() |
There was a problem hiding this comment.
Just Curious: Any particular reason we picked the unbounded cache? We may not be having fixed number of entries we want to put in the cache. So wouldn't leaving it unbounded might cause out of memory errors? It should be fine, If we are sure the number of entries are less.
There was a problem hiding this comment.
the maxmium number of entries will be (number of node) * (node config on each node) which is not that big considering that stale entries will be evicted if reaches TTL.
| } | ||
|
|
||
| @Test | ||
| public void testSetAndGetValue() { |
There was a problem hiding this comment.
Maybe 1 UT for updating the value of the key and asserting if the updated value is returned.
Issue #:
#300
Description of changes:
Tests:
tested on docker
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.