keith-turner commented on code in PR #3915:
URL: https://github.com/apache/accumulo/pull/3915#discussion_r1381058937
##########
core/src/main/java/org/apache/accumulo/core/spi/compaction/DefaultCompactionPlanner.java:
##########
@@ -146,15 +172,25 @@ public String toString() {
justification = "Field is written by Gson")
@Override
public void init(InitParameters params) {
- ExecutorConfig[] execConfigs =
- GSON.get().fromJson(params.getOptions().get("executors"),
ExecutorConfig[].class);
-
List<Executor> tmpExec = new ArrayList<>();
+ ExecutorConfig[] execConfigs = null;
+ QueueConfig[] queueConfigs = null;
+
Review Comment:
Could make the executors and queues options mutually exclusive, if both are
seen then throw an exception. If they are not mutually exclusive then could
throw an exception if they both define the same queue with differing max sizes.
##########
core/src/main/java/org/apache/accumulo/core/util/compaction/CompactionServicesConfig.java:
##########
@@ -50,16 +50,32 @@ private long getDefaultThroughput() {
.getMemoryAsBytes(Property.TSERV_COMPACTION_SERVICE_DEFAULT_RATE_LIMIT.getDefaultValue());
}
+ @SuppressWarnings("deprecation")
private Map<String,String> getConfiguration(AccumuloConfiguration aconf) {
- return
aconf.getAllPropertiesWithPrefix(Property.TSERV_COMPACTION_SERVICE_PREFIX);
+ Map<String,String> properties = new HashMap<>();
+
properties.putAll(aconf.getAllPropertiesWithPrefix(Property.TSERV_COMPACTION_SERVICE_PREFIX));
+
properties.putAll(aconf.getAllPropertiesWithPrefix(Property.COMPACTION_SERVICE_PREFIX));
+ // Return unmodifiable map
+ return Map.copyOf(properties);
}
+ @SuppressWarnings("deprecation")
public CompactionServicesConfig(AccumuloConfiguration aconf) {
Map<String,String> configs = getConfiguration(aconf);
+ String oldPrefix = Property.TSERV_COMPACTION_SERVICE_PREFIX.getKey();
+ String newPrefix = Property.COMPACTION_SERVICE_PREFIX.getKey();
Review Comment:
Seems like this could mix config for a compaction service from both
prefixes, which seems confusing. What do you think about the following
behavior?
* If a compaction service is defined in both only use properties with the
new prefix. Log something about the old prefix props being ignored.
* When a compaction service is only defined using the old prefix, then log
something about it being deprecated and use the props.
* When a compaction service is only defined using the new prefix, then use
it w/o logging anything.
##########
core/src/main/java/org/apache/accumulo/core/spi/compaction/DefaultCompactionPlanner.java:
##########
@@ -146,15 +172,25 @@ public String toString() {
justification = "Field is written by Gson")
@Override
public void init(InitParameters params) {
- ExecutorConfig[] execConfigs =
- GSON.get().fromJson(params.getOptions().get("executors"),
ExecutorConfig[].class);
-
List<Executor> tmpExec = new ArrayList<>();
+ ExecutorConfig[] execConfigs = null;
+ QueueConfig[] queueConfigs = null;
+
+ try {
+ execConfigs =
+ GSON.get().fromJson(params.getOptions().get("executors"),
ExecutorConfig[].class);
+ } catch (NullPointerException npe) {
+ // npe could result from executors not being set as properties.
+ } finally {
+ // Generated a zero-length array to avoid a npe thrown by forEach
+ if (execConfigs == null) {
+ execConfigs = new ExecutorConfig[0];
+ }
+ }
Review Comment:
Could do something like the following to avoid catching the NPE
```suggestion
if(params.getOptions().containsKey("executors)){
execConfigs =
GSON.get().fromJson(params.getOptions().get("executors"),
ExecutorConfig[].class);
}else{
execConfigs = new ExecutorConfig[0];
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
