Skip to content

Limit CS Update Task Description Size #79443

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Oct 26, 2021
Merged

Limit CS Update Task Description Size #79443

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
5e16e8d
Limit CS Update Task Description Size
original-brownbear Oct 19, 2021
c4309e9
Merge remote-tracking branch 'elastic/master' into limit-cs-update-de…
original-brownbear Oct 21, 2021
dc041cc
reuse existing logic
original-brownbear Oct 21, 2021
596e200
Merge remote-tracking branch 'elastic/master' into limit-cs-update-de…
original-brownbear Oct 26, 2021
1f3e3ab
simpler
original-brownbear Oct 26, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 24 additions & 1 deletion server/src/main/java/org/elasticsearch/cluster/ClusterStateTaskExecutor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,30 @@ default void clusterStatePublished(ClusterStatePublicationEvent clusterStatePubl
* This allows groupd task description but the submitting source.
*/
default String describeTasks(List<T> tasks) {
return String.join(", ", tasks.stream().map(t -> (CharSequence)t.toString()).filter(t -> t.length() > 0)::iterator);
final StringBuilder output = new StringBuilder();
int len = 0;
final int count = tasks.size();
int i = 0;
for (; i < count; i++) {
T task = tasks.get(i);
String t = task.toString();
if (t.length() > 0) {
if (len > 0) {
output.append(", ");
}
len += t.length();
output.append(t);
}
// don't render additional task descriptions beyond 1024 chars
if (len > 1024) {
break;
}
}
final int remaining = count - i;
if (remaining > 0) {
output.append(", ").append(remaining).append(" additional tasks");
}
return output.toString();
}

/**
Expand Down
34 changes: 29 additions & 5 deletions server/src/main/java/org/elasticsearch/cluster/service/TaskBatcher.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,14 +131,38 @@ void runIfNotProcessed(BatchedTask updateTask) {
}

if (toExecute.isEmpty() == false) {
final String tasksSummary = processTasksBySource.entrySet().stream().map(entry -> {
String tasks = updateTask.describeTasks(entry.getValue());
return tasks.isEmpty() ? entry.getKey() : entry.getKey() + "[" + tasks + "]";
}).reduce((s1, s2) -> s1 + ", " + s2).orElse("");
run(updateTask.batchingKey, toExecute, buildTasksDescription(updateTask, toExecute, processTasksBySource));
}
}
}

run(updateTask.batchingKey, toExecute, tasksSummary);
private String buildTasksDescription(BatchedTask updateTask,
Copy link
Contributor

@DaveCTurner DaveCTurner Oct 21, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Likewise here I think?

Copy link
Contributor Author

@original-brownbear original-brownbear Oct 21, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here it's a little less fun to use the general string builder than in the other case because I think it'd be nice to have the overall task count?

Copy link
Contributor

@DaveCTurner DaveCTurner Oct 21, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm confused: collectionToDelimitedStringWithLimit does yield the overall count if it truncated the output. There's no filtering happening here.

Copy link
Contributor Author

@original-brownbear original-brownbear Oct 21, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But we have this weird setup here where we have the tasks grouped by source so the counting will only work out correctly if the source is different for each task? I kinda liked having that level of detail on the counts for debugging still.

Copy link
Contributor

@DaveCTurner DaveCTurner Oct 21, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I see we use toExecute.size() rather than processTasksBySource.size().

How about either just appending (N tasks in total) if output.length() exceeds the limit we set, or else letting collectionToDelimitedStringWithLimit take some extra detail that it puts into the truncation summary?

List<BatchedTask> toExecute,
Map<String, List<BatchedTask>> processTasksBySource) {
final StringBuilder output = new StringBuilder();
int len = 0;
int count = 0;
int taskCount = 0;
for (Map.Entry<String, List<BatchedTask>> entry : processTasksBySource.entrySet()) {
String tasks = updateTask.describeTasks(entry.getValue());
final String description = tasks.isEmpty() ? entry.getKey() : entry.getKey() + "[" + tasks + "]";
if (len > 0) {
output.append(", ");
}
len += description.length();
count++;
taskCount += entry.getValue().size();
output.append(description);
// don't render additional task descriptions beyond 8k chars
if (len > 8 * 1024) {
break;
}
}
final int remaining = processTasksBySource.size() - count;
if (remaining > 0) {
output.append(", ").append(toExecute.size() - taskCount).append(" additional tasks for ").append(remaining).append(" sources");
}
return output.toString();
}

/**
Expand Down