Skip to content
This repository has been archived by the owner on Aug 2, 2022. It is now read-only.

Commit

Permalink
PR comments addressed
Browse files Browse the repository at this point in the history 
 - Add nullable annotations
 - Move default values to static final variables
  • Loading branch information
@vigyasharma
vigyasharma committed Aug 31, 2020
1 parent d72d388 commit 311db97
Show file tree
Hide file tree
Showing 7 changed files with 60 additions and 27 deletions.
13 changes: 9 additions & 4 deletions ...ro/elasticsearch/performanceanalyzer/decisionmaker/actions/configs/CacheActionConfig.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,11 @@ public class CacheActionConfig {
private ShardRequestCacheConfig shardRequestCacheConfig;
private Map<ResourceEnum, ThresholdConfig<Double>> 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<String, Object> actionConfig = conf.getActionConfigSettings();
cacheSettingsConfig = new NestedConfig("cache-settings", actionConfig);
Expand Down Expand Up @@ -86,9 +91,9 @@ private static class FieldDataCacheConfig implements ThresholdConfig<Double> {
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
Expand All @@ -110,9 +115,9 @@ private static class ShardRequestCacheConfig implements ThresholdConfig<Double>
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
Expand Down
13 changes: 9 additions & 4 deletions ...ro/elasticsearch/performanceanalyzer/decisionmaker/actions/configs/QueueActionConfig.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,11 @@ public class QueueActionConfig {
private WriteQueueConfig writeQueueConfig;
private Map<ResourceEnum, ThresholdConfig<Integer>> 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<String, Object> actionConfig = conf.getActionConfigSettings();
queueSettingsConfig = new NestedConfig("queue-settings", actionConfig);
Expand Down Expand Up @@ -85,9 +90,9 @@ private static class SearchQueueConfig implements ThresholdConfig<Integer> {
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
Expand All @@ -109,9 +114,9 @@ private static class WriteQueueConfig implements ThresholdConfig<Integer> {
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
Expand Down
5 changes: 3 additions & 2 deletions ...va/com/amazon/opendistro/elasticsearch/performanceanalyzer/rca/framework/core/Config.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand All @@ -27,11 +28,11 @@ public class Config<T> {
private String key;
private T value;

public Config(String key, Map<String, Object> parentConfig, T defaultValue, Class<? extends T> clazz) {
public Config(String key, @Nullable Map<String, Object> parentConfig, T defaultValue, Class<? extends T> clazz) {
this(key, parentConfig, defaultValue, (s) -> true, clazz);
}

public Config(String key, Map<String, Object> parentConfig, T defaultValue, Predicate<T> validator, Class<? extends T> clazz) {
public Config(String key, @Nullable Map<String, Object> parentConfig, T defaultValue, Predicate<T> validator, Class<? extends T> clazz) {
this.key = key;
this.value = defaultValue;
if (parentConfig != null) {
Expand Down
4 changes: 3 additions & 1 deletion .../amazon/opendistro/elasticsearch/performanceanalyzer/rca/framework/core/NestedConfig.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand All @@ -26,7 +27,7 @@ public class NestedConfig {
private String key;
private Map<String, Object> value;

public NestedConfig(String key, Map<String, Object> parentConfig) {
public NestedConfig(String key, @Nullable Map<String, Object> parentConfig) {
this.key = key;
this.value = null;
if (parentConfig != null) {
Expand All @@ -43,6 +44,7 @@ public String getKey() {
return key;
}

@Nullable
public Map<String, Object> getValue() {
return value;
}
Expand Down
24 changes: 16 additions & 8 deletions ...lasticsearch/performanceanalyzer/decisionmaker/actions/configs/CacheActionConfigTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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)
Expand Down
24 changes: 16 additions & 8 deletions ...lasticsearch/performanceanalyzer/decisionmaker/actions/configs/QueueActionConfigTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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)
Expand Down
4 changes: 4 additions & 0 deletions ...om/amazon/opendistro/elasticsearch/performanceanalyzer/rca/framework/core/ConfigTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,10 @@ public void testDefaults() {
Config<Integer> testInt = new Config<>("def", testConfig.getValue(), 42, Integer.class);
assertEquals("def", testInt.getKey());
assertEquals(42, (int) testInt.getValue());

Config<String> testWithNull = new Config<>("qwe", null, "test-val", String.class);
assertEquals("qwe", testWithNull.getKey());
assertEquals("test-val", testWithNull.getValue());
}

@Test
Expand Down

0 comments on commit 311db97

Please sign in to comment.