From 311db9762ebef3d3ed6951f87f271d10fc9bd266 Mon Sep 17 00:00:00 2001 From: Vigya Sharma Date: Mon, 31 Aug 2020 14:28:51 -0700 Subject: [PATCH] PR comments addressed - Add nullable annotations - Move default values to static final variables --- .../actions/configs/CacheActionConfig.java | 13 ++++++---- .../actions/configs/QueueActionConfig.java | 13 ++++++---- .../rca/framework/core/Config.java | 5 ++-- .../rca/framework/core/NestedConfig.java | 4 +++- .../configs/CacheActionConfigTest.java | 24 ++++++++++++------- .../configs/QueueActionConfigTest.java | 24 ++++++++++++------- .../rca/framework/core/ConfigTest.java | 4 ++++ 7 files changed, 60 insertions(+), 27 deletions(-) diff --git a/src/main/java/com/amazon/opendistro/elasticsearch/performanceanalyzer/decisionmaker/actions/configs/CacheActionConfig.java b/src/main/java/com/amazon/opendistro/elasticsearch/performanceanalyzer/decisionmaker/actions/configs/CacheActionConfig.java index fd26ad12c..6affa3e72 100644 --- a/src/main/java/com/amazon/opendistro/elasticsearch/performanceanalyzer/decisionmaker/actions/configs/CacheActionConfig.java +++ b/src/main/java/com/amazon/opendistro/elasticsearch/performanceanalyzer/decisionmaker/actions/configs/CacheActionConfig.java @@ -54,6 +54,11 @@ public class CacheActionConfig { private ShardRequestCacheConfig shardRequestCacheConfig; private Map> thresholdConfigMap; + public static final Double DEFAULT_FIELDDATA_CACHE_UPPER_BOUND = 0.4; + public static final Double DEFAULT_FIELDDATA_CACHE_LOWER_BOUND = 0.1; + public static final Double DEFAULT_SHARD_REQUEST_CACHE_UPPER_BOUND = 0.05; + public static final Double DEFAULT_SHARD_REQUEST_CACHE_LOWER_BOUND = 0.01; + public CacheActionConfig(RcaConf conf) { Map actionConfig = conf.getActionConfigSettings(); cacheSettingsConfig = new NestedConfig("cache-settings", actionConfig); @@ -86,9 +91,9 @@ private static class FieldDataCacheConfig implements ThresholdConfig { public FieldDataCacheConfig(NestedConfig cacheSettingsConfig) { NestedConfig fieldDataCacheConfig = new NestedConfig("fielddata", cacheSettingsConfig.getValue()); fieldDataCacheUpperBound = new Config<>("upper-bound", fieldDataCacheConfig.getValue(), - 0.4, (s) -> (s > 0), Double.class); + DEFAULT_FIELDDATA_CACHE_UPPER_BOUND, (s) -> (s > 0), Double.class); fieldDataCacheLowerBound = new Config<>("lower-bound", fieldDataCacheConfig.getValue(), - 0.1, (s) -> (s > 0), Double.class); + DEFAULT_FIELDDATA_CACHE_LOWER_BOUND, (s) -> (s > 0), Double.class); } @Override @@ -110,9 +115,9 @@ private static class ShardRequestCacheConfig implements ThresholdConfig public ShardRequestCacheConfig(NestedConfig cacheSettingsConfig) { NestedConfig shardRequestCacheConfig = new NestedConfig("shard-request", cacheSettingsConfig.getValue()); shardRequestCacheUpperBound = new Config<>("upper-bound", shardRequestCacheConfig.getValue(), - 0.05, (s) -> (s > 0), Double.class); + DEFAULT_SHARD_REQUEST_CACHE_UPPER_BOUND, (s) -> (s > 0), Double.class); shardRequestCacheLowerBound = new Config<>("lower-bound", shardRequestCacheConfig.getValue(), - 0.01, (s) -> (s > 0), Double.class); + DEFAULT_SHARD_REQUEST_CACHE_LOWER_BOUND, (s) -> (s > 0), Double.class); } @Override diff --git a/src/main/java/com/amazon/opendistro/elasticsearch/performanceanalyzer/decisionmaker/actions/configs/QueueActionConfig.java b/src/main/java/com/amazon/opendistro/elasticsearch/performanceanalyzer/decisionmaker/actions/configs/QueueActionConfig.java index 3d996a7eb..faa5bc58e 100644 --- a/src/main/java/com/amazon/opendistro/elasticsearch/performanceanalyzer/decisionmaker/actions/configs/QueueActionConfig.java +++ b/src/main/java/com/amazon/opendistro/elasticsearch/performanceanalyzer/decisionmaker/actions/configs/QueueActionConfig.java @@ -53,6 +53,11 @@ public class QueueActionConfig { private WriteQueueConfig writeQueueConfig; private Map> thresholdConfigMap; + public static final int DEFAULT_SEARCH_QUEUE_UPPER_BOUND = 3000; + public static final int DEFAULT_SEARCH_QUEUE_LOWER_BOUND = 500; + public static final int DEFAULT_WRITE_QUEUE_UPPER_BOUND = 1000; + public static final int DEFAULT_WRITE_QUEUE_LOWER_BOUND = 50; + public QueueActionConfig(RcaConf conf) { Map actionConfig = conf.getActionConfigSettings(); queueSettingsConfig = new NestedConfig("queue-settings", actionConfig); @@ -85,9 +90,9 @@ private static class SearchQueueConfig implements ThresholdConfig { public SearchQueueConfig(NestedConfig queueSettingsConfig) { NestedConfig searchQueueConfig = new NestedConfig("search", queueSettingsConfig.getValue()); searchQueueUpperBound = new Config<>("upper-bound", searchQueueConfig.getValue(), - 3000, (s) -> (s >= 0), Integer.class); + DEFAULT_SEARCH_QUEUE_UPPER_BOUND, (s) -> (s >= 0), Integer.class); searchQueueLowerBound = new Config<>("lower-bound", searchQueueConfig.getValue(), - 500, (s) -> (s >= 0), Integer.class); + DEFAULT_SEARCH_QUEUE_LOWER_BOUND, (s) -> (s >= 0), Integer.class); } @Override @@ -109,9 +114,9 @@ private static class WriteQueueConfig implements ThresholdConfig { public WriteQueueConfig(NestedConfig queueSettingsConfig) { NestedConfig writeQueueConfig = new NestedConfig("write", queueSettingsConfig.getValue()); writeQueueUpperBound = new Config<>("upper-bound", writeQueueConfig.getValue(), - 1000, (s) -> (s >= 0), Integer.class); + DEFAULT_WRITE_QUEUE_UPPER_BOUND, (s) -> (s >= 0), Integer.class); writeQueueLowerBound = new Config<>("lower-bound", writeQueueConfig.getValue(), - 50, (s) -> (s >= 0), Integer.class); + DEFAULT_WRITE_QUEUE_LOWER_BOUND, (s) -> (s >= 0), Integer.class); } @Override diff --git a/src/main/java/com/amazon/opendistro/elasticsearch/performanceanalyzer/rca/framework/core/Config.java b/src/main/java/com/amazon/opendistro/elasticsearch/performanceanalyzer/rca/framework/core/Config.java index caa8e83a8..4bab5818b 100644 --- a/src/main/java/com/amazon/opendistro/elasticsearch/performanceanalyzer/rca/framework/core/Config.java +++ b/src/main/java/com/amazon/opendistro/elasticsearch/performanceanalyzer/rca/framework/core/Config.java @@ -17,6 +17,7 @@ import java.util.Map; import java.util.function.Predicate; +import javax.annotation.Nullable; import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; @@ -27,11 +28,11 @@ public class Config { private String key; private T value; - public Config(String key, Map parentConfig, T defaultValue, Class clazz) { + public Config(String key, @Nullable Map parentConfig, T defaultValue, Class clazz) { this(key, parentConfig, defaultValue, (s) -> true, clazz); } - public Config(String key, Map parentConfig, T defaultValue, Predicate validator, Class clazz) { + public Config(String key, @Nullable Map parentConfig, T defaultValue, Predicate validator, Class clazz) { this.key = key; this.value = defaultValue; if (parentConfig != null) { diff --git a/src/main/java/com/amazon/opendistro/elasticsearch/performanceanalyzer/rca/framework/core/NestedConfig.java b/src/main/java/com/amazon/opendistro/elasticsearch/performanceanalyzer/rca/framework/core/NestedConfig.java index c8fc96c82..7b2628749 100644 --- a/src/main/java/com/amazon/opendistro/elasticsearch/performanceanalyzer/rca/framework/core/NestedConfig.java +++ b/src/main/java/com/amazon/opendistro/elasticsearch/performanceanalyzer/rca/framework/core/NestedConfig.java @@ -16,6 +16,7 @@ package com.amazon.opendistro.elasticsearch.performanceanalyzer.rca.framework.core; import java.util.Map; +import javax.annotation.Nullable; import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; @@ -26,7 +27,7 @@ public class NestedConfig { private String key; private Map value; - public NestedConfig(String key, Map parentConfig) { + public NestedConfig(String key, @Nullable Map parentConfig) { this.key = key; this.value = null; if (parentConfig != null) { @@ -43,6 +44,7 @@ public String getKey() { return key; } + @Nullable public Map getValue() { return value; } diff --git a/src/test/java/com/amazon/opendistro/elasticsearch/performanceanalyzer/decisionmaker/actions/configs/CacheActionConfigTest.java b/src/test/java/com/amazon/opendistro/elasticsearch/performanceanalyzer/decisionmaker/actions/configs/CacheActionConfigTest.java index 0e7c23f34..a91f494a8 100644 --- a/src/test/java/com/amazon/opendistro/elasticsearch/performanceanalyzer/decisionmaker/actions/configs/CacheActionConfigTest.java +++ b/src/test/java/com/amazon/opendistro/elasticsearch/performanceanalyzer/decisionmaker/actions/configs/CacheActionConfigTest.java @@ -55,10 +55,14 @@ public void testDefaults() throws Exception { RcaConf conf = new RcaConf(); conf.readConfigFromString(configStr); CacheActionConfig cacheActionConfig = new CacheActionConfig(conf); - assertEquals(0.4, cacheActionConfig.getThresholdConfig(ResourceEnum.FIELD_DATA_CACHE).upperBound(), 0.00001); - assertEquals(0.1, cacheActionConfig.getThresholdConfig(ResourceEnum.FIELD_DATA_CACHE).lowerBound(), 0.00001); - assertEquals(0.05, cacheActionConfig.getThresholdConfig(ResourceEnum.SHARD_REQUEST_CACHE).upperBound(), 0.00001); - assertEquals(0.01, cacheActionConfig.getThresholdConfig(ResourceEnum.SHARD_REQUEST_CACHE).lowerBound(), 0.00001); + assertEquals(CacheActionConfig.DEFAULT_FIELDDATA_CACHE_UPPER_BOUND, + cacheActionConfig.getThresholdConfig(ResourceEnum.FIELD_DATA_CACHE).upperBound(), 0.00001); + assertEquals(CacheActionConfig.DEFAULT_FIELDDATA_CACHE_LOWER_BOUND, + cacheActionConfig.getThresholdConfig(ResourceEnum.FIELD_DATA_CACHE).lowerBound(), 0.00001); + assertEquals(CacheActionConfig.DEFAULT_SHARD_REQUEST_CACHE_UPPER_BOUND, + cacheActionConfig.getThresholdConfig(ResourceEnum.SHARD_REQUEST_CACHE).upperBound(), 0.00001); + assertEquals(CacheActionConfig.DEFAULT_SHARD_REQUEST_CACHE_LOWER_BOUND, + cacheActionConfig.getThresholdConfig(ResourceEnum.SHARD_REQUEST_CACHE).lowerBound(), 0.00001); } @Test @@ -83,10 +87,14 @@ public void testInvalidConfigValues() throws Exception { // Invalid values in config, should resolve back to defaults CacheActionConfig cacheActionConfig = new CacheActionConfig(conf); - assertEquals(0.4, cacheActionConfig.getThresholdConfig(ResourceEnum.FIELD_DATA_CACHE).upperBound(), 0.00001); - assertEquals(0.1, cacheActionConfig.getThresholdConfig(ResourceEnum.FIELD_DATA_CACHE).lowerBound(), 0.00001); - assertEquals(0.05, cacheActionConfig.getThresholdConfig(ResourceEnum.SHARD_REQUEST_CACHE).upperBound(), 0.00001); - assertEquals(0.01, cacheActionConfig.getThresholdConfig(ResourceEnum.SHARD_REQUEST_CACHE).lowerBound(), 0.00001); + assertEquals(CacheActionConfig.DEFAULT_FIELDDATA_CACHE_UPPER_BOUND, + cacheActionConfig.getThresholdConfig(ResourceEnum.FIELD_DATA_CACHE).upperBound(), 0.00001); + assertEquals(CacheActionConfig.DEFAULT_FIELDDATA_CACHE_LOWER_BOUND, + cacheActionConfig.getThresholdConfig(ResourceEnum.FIELD_DATA_CACHE).lowerBound(), 0.00001); + assertEquals(CacheActionConfig.DEFAULT_SHARD_REQUEST_CACHE_UPPER_BOUND, + cacheActionConfig.getThresholdConfig(ResourceEnum.SHARD_REQUEST_CACHE).upperBound(), 0.00001); + assertEquals(CacheActionConfig.DEFAULT_SHARD_REQUEST_CACHE_LOWER_BOUND, + cacheActionConfig.getThresholdConfig(ResourceEnum.SHARD_REQUEST_CACHE).lowerBound(), 0.00001); } @Test(expected = IllegalArgumentException.class) diff --git a/src/test/java/com/amazon/opendistro/elasticsearch/performanceanalyzer/decisionmaker/actions/configs/QueueActionConfigTest.java b/src/test/java/com/amazon/opendistro/elasticsearch/performanceanalyzer/decisionmaker/actions/configs/QueueActionConfigTest.java index bc0adb672..a7959e000 100644 --- a/src/test/java/com/amazon/opendistro/elasticsearch/performanceanalyzer/decisionmaker/actions/configs/QueueActionConfigTest.java +++ b/src/test/java/com/amazon/opendistro/elasticsearch/performanceanalyzer/decisionmaker/actions/configs/QueueActionConfigTest.java @@ -55,10 +55,14 @@ public void testDefaults() throws Exception { RcaConf conf = new RcaConf(); conf.readConfigFromString(configStr); QueueActionConfig queueActionConfig = new QueueActionConfig(conf); - assertEquals(3000, (int) queueActionConfig.getThresholdConfig(ResourceEnum.SEARCH_THREADPOOL).upperBound()); - assertEquals(500, (int) queueActionConfig.getThresholdConfig(ResourceEnum.SEARCH_THREADPOOL).lowerBound()); - assertEquals(1000, (int) queueActionConfig.getThresholdConfig(ResourceEnum.WRITE_THREADPOOL).upperBound()); - assertEquals(50, (int) queueActionConfig.getThresholdConfig(ResourceEnum.WRITE_THREADPOOL).lowerBound()); + assertEquals(QueueActionConfig.DEFAULT_SEARCH_QUEUE_UPPER_BOUND, + (int) queueActionConfig.getThresholdConfig(ResourceEnum.SEARCH_THREADPOOL).upperBound()); + assertEquals(QueueActionConfig.DEFAULT_SEARCH_QUEUE_LOWER_BOUND, + (int) queueActionConfig.getThresholdConfig(ResourceEnum.SEARCH_THREADPOOL).lowerBound()); + assertEquals(QueueActionConfig.DEFAULT_WRITE_QUEUE_UPPER_BOUND, + (int) queueActionConfig.getThresholdConfig(ResourceEnum.WRITE_THREADPOOL).upperBound()); + assertEquals(QueueActionConfig.DEFAULT_WRITE_QUEUE_LOWER_BOUND, + (int) queueActionConfig.getThresholdConfig(ResourceEnum.WRITE_THREADPOOL).lowerBound()); } @Test @@ -83,10 +87,14 @@ public void testInvalidConfigValues() throws Exception { // Invalid values in config, should resolve back to defaults QueueActionConfig queueActionConfig = new QueueActionConfig(conf); - assertEquals(3000, (int) queueActionConfig.getThresholdConfig(ResourceEnum.SEARCH_THREADPOOL).upperBound()); - assertEquals(500, (int) queueActionConfig.getThresholdConfig(ResourceEnum.SEARCH_THREADPOOL).lowerBound()); - assertEquals(1000, (int) queueActionConfig.getThresholdConfig(ResourceEnum.WRITE_THREADPOOL).upperBound()); - assertEquals(50, (int) queueActionConfig.getThresholdConfig(ResourceEnum.WRITE_THREADPOOL).lowerBound()); + assertEquals(QueueActionConfig.DEFAULT_SEARCH_QUEUE_UPPER_BOUND, + (int) queueActionConfig.getThresholdConfig(ResourceEnum.SEARCH_THREADPOOL).upperBound()); + assertEquals(QueueActionConfig.DEFAULT_SEARCH_QUEUE_LOWER_BOUND, + (int) queueActionConfig.getThresholdConfig(ResourceEnum.SEARCH_THREADPOOL).lowerBound()); + assertEquals(QueueActionConfig.DEFAULT_WRITE_QUEUE_UPPER_BOUND, + (int) queueActionConfig.getThresholdConfig(ResourceEnum.WRITE_THREADPOOL).upperBound()); + assertEquals(QueueActionConfig.DEFAULT_WRITE_QUEUE_LOWER_BOUND, + (int) queueActionConfig.getThresholdConfig(ResourceEnum.WRITE_THREADPOOL).lowerBound()); } @Test(expected = IllegalArgumentException.class) diff --git a/src/test/java/com/amazon/opendistro/elasticsearch/performanceanalyzer/rca/framework/core/ConfigTest.java b/src/test/java/com/amazon/opendistro/elasticsearch/performanceanalyzer/rca/framework/core/ConfigTest.java index 91da92d92..107f7e6dc 100644 --- a/src/test/java/com/amazon/opendistro/elasticsearch/performanceanalyzer/rca/framework/core/ConfigTest.java +++ b/src/test/java/com/amazon/opendistro/elasticsearch/performanceanalyzer/rca/framework/core/ConfigTest.java @@ -71,6 +71,10 @@ public void testDefaults() { Config testInt = new Config<>("def", testConfig.getValue(), 42, Integer.class); assertEquals("def", testInt.getKey()); assertEquals(42, (int) testInt.getValue()); + + Config testWithNull = new Config<>("qwe", null, "test-val", String.class); + assertEquals("qwe", testWithNull.getKey()); + assertEquals("test-val", testWithNull.getValue()); } @Test