Skip to content

Commit a1e1ec4

Browse files
ywelschywelsch
committed
address review comments
1 parent abe574f commit a1e1ec4

File tree

8 files changed

+163
-137
lines changed

8 files changed

+163
-137
lines changed

‎core/src/main/java/org/elasticsearch/cluster/ClusterStateTaskExecutor.java‎

Lines changed: 20 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -18,14 +18,13 @@
1818
*/
1919
package org.elasticsearch.cluster;
2020

21-
import org.elasticsearch.cluster.service.TaskBatching;
2221
import org.elasticsearch.common.Nullable;
2322

2423
import java.util.IdentityHashMap;
2524
import java.util.List;
2625
import java.util.Map;
2726

28-
public interface ClusterStateTaskExecutor<T> extends TaskBatching.BatchingKey<T> {
27+
public interface ClusterStateTaskExecutor<T> {
2928
/**
3029
* Update the cluster state based on the current state and the given tasks. Return the *same instance* if no state
3130
* should be changed.
@@ -48,6 +47,25 @@ default boolean runOnlyOnMaster() {
4847
default void clusterStatePublished(ClusterChangedEvent clusterChangedEvent) {
4948
}
5049

50+
/**
51+
* Builds a concise description of a list of tasks (to be used in logging etc.).
52+
*
53+
* Note that the tasks given are not necessarily the same as those that will be passed to {@link #execute(ClusterState, List)}.
54+
* but are guaranteed to be a subset of them. This method can be called multiple times with different lists before execution.
55+
* This allows groupd task description but the submitting source.
56+
*/
57+
default String describeTasks(List<T> tasks) {
58+
return tasks.stream().map(T::toString).reduce((s1,s2) -> {
59+
if (s1.isEmpty()) {
60+
return s2;
61+
} else if (s2.isEmpty()) {
62+
return s1;
63+
} else {
64+
return s1 + ", " + s2;
65+
}
66+
}).orElse("");
67+
}
68+
5169
/**
5270
* Represents the result of a batched execution of cluster state update tasks
5371
* @param <T> the type of the cluster state update task

‎core/src/main/java/org/elasticsearch/cluster/service/ClusterService.java‎

Lines changed: 38 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -78,6 +78,7 @@
7878
import java.util.Queue;
7979
import java.util.concurrent.ConcurrentHashMap;
8080
import java.util.concurrent.CopyOnWriteArrayList;
81+
import java.util.concurrent.Executor;
8182
import java.util.concurrent.Future;
8283
import java.util.concurrent.ScheduledFuture;
8384
import java.util.concurrent.TimeUnit;
@@ -109,7 +110,7 @@ public class ClusterService extends AbstractLifecycleComponent {
109110
private TimeValue slowTaskLoggingThreshold;
110111

111112
private volatile PrioritizedEsThreadPoolExecutor threadPoolExecutor;
112-
private volatile ClusterServiceTaskBatching taskBatching;
113+
private volatile ClusterServiceTaskBatcher taskBatching;
113114

114115
/**
115116
* Those 3 state listeners are changing infrequently - CopyOnWriteArrayList is just fine
@@ -215,8 +216,8 @@ protected synchronized void doStart() {
215216
return ClusterState.builder(state).nodes(nodes).blocks(initialBlocks).build();
216217
});
217218
this.threadPoolExecutor = EsExecutors.newSinglePrioritizing(UPDATE_THREAD_NAME,
218-
daemonThreadFactory(settings, UPDATE_THREAD_NAME), threadPool.getThreadContext());
219-
this.taskBatching = new ClusterServiceTaskBatching(logger, threadPoolExecutor, threadPool);
219+
daemonThreadFactory(settings, UPDATE_THREAD_NAME), threadPool.getThreadContext(), threadPool.scheduler());
220+
this.taskBatching = new ClusterServiceTaskBatcher(logger, threadPoolExecutor);
220221
}
221222

222223
@Override
@@ -240,32 +241,41 @@ protected synchronized void doStop() {
240241
protected synchronized void doClose() {
241242
}
242243

243-
class ClusterServiceTaskBatching extends TaskBatching {
244+
class ClusterServiceTaskBatcher extends TaskBatcher {
244245

245-
ClusterServiceTaskBatching(Logger logger, PrioritizedEsThreadPoolExecutor threadExecutor, ThreadPool threadPool) {
246-
super(logger, threadExecutor, threadPool);
246+
ClusterServiceTaskBatcher(Logger logger, PrioritizedEsThreadPoolExecutor threadExecutor) {
247+
super(logger, threadExecutor);
247248
}
248249

249250
@Override
250-
protected void onTimeout(BatchingTask task, TimeValue timeout) {
251-
((UpdateTask) task).listener.onFailure(task.source, new ProcessClusterEventTimeoutException(timeout, task.source));
251+
protected void onTimeout(List<? extends BatchedTask> tasks, TimeValue timeout) {
252+
threadPool.generic().execute(
253+
() -> tasks.forEach(
254+
task -> ((UpdateTask) task).listener.onFailure(task.source,
255+
new ProcessClusterEventTimeoutException(timeout, task.source))));
252256
}
253257

254258
@Override
255-
protected void run(Object batchingKey, List<? extends BatchingTask> tasks, String tasksSummary) {
259+
protected void run(Object batchingKey, List<? extends BatchedTask> tasks, String tasksSummary) {
256260
ClusterStateTaskExecutor<Object> taskExecutor = (ClusterStateTaskExecutor<Object>) batchingKey;
257261
List<UpdateTask> updateTasks = (List<UpdateTask>) tasks;
258262
runTasks(new ClusterService.TaskInputs(taskExecutor, updateTasks, tasksSummary));
259263
}
260264

261-
class UpdateTask extends BatchingTask {
265+
class UpdateTask extends BatchedTask {
262266
final ClusterStateTaskListener listener;
263267

264268
UpdateTask(Priority priority, String source, Object task, ClusterStateTaskListener listener,
265269
ClusterStateTaskExecutor<?> executor) {
266270
super(priority, source, executor, task);
267271
this.listener = listener;
268272
}
273+
274+
@Override
275+
public String describeTasks(List<? extends BatchedTask> tasks) {
276+
return ((ClusterStateTaskExecutor<Object>) batchingKey).describeTasks(
277+
tasks.stream().map(BatchedTask::getTask).collect(Collectors.toList()));
278+
}
269279
}
270280
}
271281

@@ -459,7 +469,7 @@ public <T> void submitStateUpdateTasks(final String source,
459469
return;
460470
}
461471
try {
462-
List<ClusterServiceTaskBatching.UpdateTask> safeTasks = tasks.entrySet().stream()
472+
List<ClusterServiceTaskBatcher.UpdateTask> safeTasks = tasks.entrySet().stream()
463473
.map(e -> taskBatching.new UpdateTask(config.priority(), source, e.getKey(), safe(e.getValue(), logger), executor))
464474
.collect(Collectors.toList());
465475
taskBatching.submitTasks(safeTasks, config.timeout());
@@ -600,11 +610,11 @@ void runTasks(TaskInputs taskInputs) {
600610
public TaskOutputs calculateTaskOutputs(TaskInputs taskInputs, ClusterState previousClusterState, long startTimeNS) {
601611
ClusterTasksResult<Object> clusterTasksResult = executeTasks(taskInputs, startTimeNS, previousClusterState);
602612
// extract those that are waiting for results
603-
List<ClusterServiceTaskBatching.UpdateTask> nonFailedTasks = new ArrayList<>();
604-
for (ClusterServiceTaskBatching.UpdateTask updateTask : taskInputs.updateTasks) {
605-
assert clusterTasksResult.executionResults.containsKey(updateTask.taskIdentity) : "missing " + updateTask;
613+
List<ClusterServiceTaskBatcher.UpdateTask> nonFailedTasks = new ArrayList<>();
614+
for (ClusterServiceTaskBatcher.UpdateTask updateTask : taskInputs.updateTasks) {
615+
assert clusterTasksResult.executionResults.containsKey(updateTask.task) : "missing " + updateTask;
606616
final ClusterStateTaskExecutor.TaskResult taskResult =
607-
clusterTasksResult.executionResults.get(updateTask.taskIdentity);
617+
clusterTasksResult.executionResults.get(updateTask.task);
608618
if (taskResult.isSuccess()) {
609619
nonFailedTasks.add(updateTask);
610620
}
@@ -619,7 +629,7 @@ private ClusterTasksResult<Object> executeTasks(TaskInputs taskInputs, long star
619629
ClusterTasksResult<Object> clusterTasksResult;
620630
try {
621631
List<Object> inputs = taskInputs.updateTasks.stream()
622-
.map(ClusterServiceTaskBatching.UpdateTask::getTaskIdentity).collect(Collectors.toList());
632+
.map(ClusterServiceTaskBatcher.UpdateTask::getTask).collect(Collectors.toList());
623633
clusterTasksResult = taskInputs.executor.execute(previousClusterState, inputs);
624634
} catch (Exception e) {
625635
TimeValue executionTime = TimeValue.timeValueMillis(Math.max(0, TimeValue.nsecToMSec(currentTimeInNanos() - startTimeNS)));
@@ -637,7 +647,7 @@ private ClusterTasksResult<Object> executeTasks(TaskInputs taskInputs, long star
637647
}
638648
warnAboutSlowTaskIfNeeded(executionTime, taskInputs.summary);
639649
clusterTasksResult = ClusterTasksResult.builder()
640-
.failures(taskInputs.updateTasks.stream().map(ClusterServiceTaskBatching.UpdateTask::getTaskIdentity)::iterator, e)
650+
.failures(taskInputs.updateTasks.stream().map(ClusterServiceTaskBatcher.UpdateTask::getTask)::iterator, e)
641651
.build(previousClusterState);
642652
}
643653

@@ -648,8 +658,8 @@ private ClusterTasksResult<Object> executeTasks(TaskInputs taskInputs, long star
648658
boolean assertsEnabled = false;
649659
assert (assertsEnabled = true);
650660
if (assertsEnabled) {
651-
for (ClusterServiceTaskBatching.UpdateTask updateTask : taskInputs.updateTasks) {
652-
assert clusterTasksResult.executionResults.containsKey(updateTask.taskIdentity) :
661+
for (ClusterServiceTaskBatcher.UpdateTask updateTask : taskInputs.updateTasks) {
662+
assert clusterTasksResult.executionResults.containsKey(updateTask.task) :
653663
"missing task result for " + updateTask;
654664
}
655665
}
@@ -814,10 +824,10 @@ private void callClusterStateAppliers(ClusterState newClusterState, ClusterChang
814824
*/
815825
class TaskInputs {
816826
public final String summary;
817-
public final List<ClusterServiceTaskBatching.UpdateTask> updateTasks;
827+
public final List<ClusterServiceTaskBatcher.UpdateTask> updateTasks;
818828
public final ClusterStateTaskExecutor<Object> executor;
819829

820-
TaskInputs(ClusterStateTaskExecutor<Object> executor, List<ClusterServiceTaskBatching.UpdateTask> updateTasks, String summary) {
830+
TaskInputs(ClusterStateTaskExecutor<Object> executor, List<ClusterServiceTaskBatcher.UpdateTask> updateTasks, String summary) {
821831
this.summary = summary;
822832
this.executor = executor;
823833
this.updateTasks = updateTasks;
@@ -839,11 +849,11 @@ class TaskOutputs {
839849
public final TaskInputs taskInputs;
840850
public final ClusterState previousClusterState;
841851
public final ClusterState newClusterState;
842-
public final List<ClusterServiceTaskBatching.UpdateTask> nonFailedTasks;
852+
public final List<ClusterServiceTaskBatcher.UpdateTask> nonFailedTasks;
843853
public final Map<Object, ClusterStateTaskExecutor.TaskResult> executionResults;
844854

845855
TaskOutputs(TaskInputs taskInputs, ClusterState previousClusterState,
846-
ClusterState newClusterState, List<ClusterServiceTaskBatching.UpdateTask> nonFailedTasks,
856+
ClusterState newClusterState, List<ClusterServiceTaskBatcher.UpdateTask> nonFailedTasks,
847857
Map<Object, ClusterStateTaskExecutor.TaskResult> executionResults) {
848858
this.taskInputs = taskInputs;
849859
this.previousClusterState = previousClusterState;
@@ -895,9 +905,9 @@ public boolean clusterStateUnchanged() {
895905

896906
public void notifyFailedTasks() {
897907
// fail all tasks that have failed
898-
for (ClusterServiceTaskBatching.UpdateTask updateTask : taskInputs.updateTasks) {
899-
assert executionResults.containsKey(updateTask.taskIdentity) : "missing " + updateTask;
900-
final ClusterStateTaskExecutor.TaskResult taskResult = executionResults.get(updateTask.taskIdentity);
908+
for (ClusterServiceTaskBatcher.UpdateTask updateTask : taskInputs.updateTasks) {
909+
assert executionResults.containsKey(updateTask.task) : "missing " + updateTask;
910+
final ClusterStateTaskExecutor.TaskResult taskResult = executionResults.get(updateTask.task);
901911
if (taskResult.isSuccess() == false) {
902912
updateTask.listener.onFailure(updateTask.source, taskResult.getFailure());
903913
}
@@ -1065,7 +1075,7 @@ public void clusterChanged(ClusterChangedEvent event) {
10651075
if (!master && event.localNodeMaster()) {
10661076
master = true;
10671077
for (LocalNodeMasterListener listener : listeners) {
1068-
java.util.concurrent.Executor executor = threadPool.executor(listener.executorName());
1078+
Executor executor = threadPool.executor(listener.executorName());
10691079
executor.execute(new OnMasterRunnable(listener));
10701080
}
10711081
return;
@@ -1074,7 +1084,7 @@ public void clusterChanged(ClusterChangedEvent event) {
10741084
if (master && !event.localNodeMaster()) {
10751085
master = false;
10761086
for (LocalNodeMasterListener listener : listeners) {
1077-
java.util.concurrent.Executor executor = threadPool.executor(listener.executorName());
1087+
Executor executor = threadPool.executor(listener.executorName());
10781088
executor.execute(new OffMasterRunnable(listener));
10791089
}
10801090
}

0 commit comments

Comments
 (0)