ddanielr commented on code in PR #3915:
URL: https://github.com/apache/accumulo/pull/3915#discussion_r1402908367
##########
core/src/main/java/org/apache/accumulo/core/spi/compaction/DefaultCompactionPlanner.java:
##########
@@ -147,44 +186,78 @@ 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<>();
+ String values;
+
+ if (params.getOptions().containsKey("executors")
+ && !params.getOptions().get("executors").isBlank()) {
+ values = params.getOptions().get("executors");
- for (ExecutorConfig executorConfig : execConfigs) {
- Long maxSize = executorConfig.maxSize == null ? null
- :
ConfigurationTypeHelper.getFixedMemoryAsBytes(executorConfig.maxSize);
+ // Generate a list of fields from the desired object.
+ final List<String> execFields =
Arrays.stream(ExecutorConfig.class.getDeclaredFields())
+ .map(Field::getName).collect(Collectors.toList());
- CompactionExecutorId ceid;
+ for (JsonElement element : GSON.get().fromJson(values, JsonArray.class))
{
+ validateConfig(element, execFields, ExecutorConfig.class.getName());
+ ExecutorConfig executorConfig = GSON.get().fromJson(element,
ExecutorConfig.class);
- // If not supplied, GSON will leave type null. Default to internal
- if (executorConfig.type == null) {
- executorConfig.type = "internal";
+ Long maxSize = executorConfig.maxSize == null ? null
+ :
ConfigurationTypeHelper.getFixedMemoryAsBytes(executorConfig.maxSize);
+ CompactionExecutorId ceid;
+
+ // If not supplied, GSON will leave type null. Default to internal
+ if (executorConfig.type == null) {
+ executorConfig.type = "internal";
+ }
+
+ switch (executorConfig.type) {
+ case "internal":
+ Preconditions.checkArgument(null == executorConfig.queue,
+ "'queue' should not be specified for internal compactions");
+ int numThreads = Objects.requireNonNull(executorConfig.numThreads,
+ "'numThreads' must be specified for internal type");
+ ceid =
params.getExecutorManager().createExecutor(executorConfig.name, numThreads);
+ break;
+ case "external":
+ Preconditions.checkArgument(null == executorConfig.numThreads,
+ "'numThreads' should not be specified for external
compactions");
+ String queue = Objects.requireNonNull(executorConfig.queue,
+ "'queue' must be specified for external type");
+ ceid = params.getExecutorManager().getExternalExecutor(queue);
+ break;
+ default:
+ throw new IllegalArgumentException("type must be 'internal' or
'external'");
+ }
+ tmpExec.add(new Executor(ceid, maxSize));
}
+ }
- switch (executorConfig.type) {
- case "internal":
- Preconditions.checkArgument(null == executorConfig.queue,
- "'queue' should not be specified for internal compactions");
- int numThreads = Objects.requireNonNull(executorConfig.numThreads,
- "'numThreads' must be specified for internal type");
- ceid =
params.getExecutorManager().createExecutor(executorConfig.name, numThreads);
- break;
- case "external":
- Preconditions.checkArgument(null == executorConfig.numThreads,
- "'numThreads' should not be specified for external compactions");
- String queue = Objects.requireNonNull(executorConfig.queue,
- "'queue' must be specified for external type");
- ceid = params.getExecutorManager().getExternalExecutor(queue);
- break;
- default:
- throw new IllegalArgumentException("type must be 'internal' or
'external'");
+ if (params.getOptions().containsKey("queues") &&
!params.getOptions().get("queues").isBlank()) {
+ values = params.getOptions().get("queues");
+
+ // Generate a list of fields from the desired object.
+ final List<String> queueFields =
Arrays.stream(QueueConfig.class.getDeclaredFields())
+ .map(Field::getName).collect(Collectors.toList());
+
+ for (JsonElement element : GSON.get().fromJson(values, JsonArray.class))
{
+ validateConfig(element, queueFields, QueueConfig.class.getName());
+ QueueConfig queueConfig = GSON.get().fromJson(element,
QueueConfig.class);
+
+ Long maxSize = queueConfig.maxSize == null ? null
+ :
ConfigurationTypeHelper.getFixedMemoryAsBytes(queueConfig.maxSize);
+
+ CompactionExecutorId ceid;
+ String queue = Objects.requireNonNull(queueConfig.name, "'name' must
be specified");
+ ceid = params.getExecutorManager().getExternalExecutor(queue);
+ tmpExec.add(new Executor(ceid, maxSize));
}
- tmpExec.add(new Executor(ceid, maxSize));
}
- Collections.sort(tmpExec, Comparator.comparing(Executor::getMaxSize,
+ if (tmpExec.size() < 1) {
Review Comment:
I pulled the check code over to 2.1 and I don't think there is a case where
no executors would exist without the planner already failing with a
NullPointerException.
If `executors` is set to a blank string: `executors=""` then a NPE is thrown
by the "for each" loop.
`for (ExecutorConfig executorConfig : execConfigs) {`
If `executors` is set to a empty json array: `executors="[{}]"` then it
does pass the "for each" loop NPE condition.
Since `executorconfig.type` is null, it defaults to a `internal` executor
and then an NPE is thrown by the
`Objects.requireNonNull(executorConfig.numThreads,...` check.
So, I'd have to modify the code to check for a blank `executors` string to
exist and allow that state. Then I could convert the error over to a
IllegalStateException vs the current NPE.
--
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]
