Skip to content

Commit b0f3c13

Browse files
DaveCTurnerDaveCTurner
committed
Introduce BoundedDelimitedStringCollector
An issue with `Strings#collectionToDelimitedStringWithLimit` is that you need to collect all the items together up front first, even if you're going to throw most of them away. This commit introduces `BoundedDelimitedStringCollector` which allows to accumulate the items one-at-a-time instead.
1 parent b218f69 commit b0f3c13

File tree

6 files changed

+225
-123
lines changed

6 files changed

+225
-123
lines changed

‎server/src/main/java/org/elasticsearch/action/admin/indices/rollover/LazyRolloverAction.java‎

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -188,7 +188,8 @@ record LazyRolloverExecutor(
188188
@Override
189189
public ClusterState execute(BatchExecutionContext<LazyRolloverTask> batchExecutionContext) {
190190
final var listener = new AllocationActionMultiListener<RolloverResponse>(threadPool.getThreadContext());
191-
final var results = new ArrayList<String>(batchExecutionContext.taskContexts().size());
191+
final var reasonBuilder = new StringBuilder();
192+
final var resultsCollector = new Strings.BoundedDelimitedStringCollector(reasonBuilder, "lazy bulk rollover [", ",", "]", 1024);
192193
var state = batchExecutionContext.initialState();
193194
Map<ProjectId, Map<RolloverRequest, List<TaskContext<LazyRolloverTask>>>> groupedRequests = new HashMap<>();
194195
for (final var taskContext : batchExecutionContext.taskContexts()) {
@@ -202,7 +203,7 @@ public ClusterState execute(BatchExecutionContext<LazyRolloverTask> batchExecuti
202203
try {
203204
RolloverRequest rolloverRequest = entry.getKey();
204205
final var projectState = state.projectState(projectRequests.getKey());
205-
state = executeTask(projectState, rolloverRequest, results, rolloverTaskContexts, listener);
206+
state = executeTask(projectState, rolloverRequest, resultsCollector::appendItem, rolloverTaskContexts, listener);
206207
} catch (Exception e) {
207208
rolloverTaskContexts.forEach(taskContext -> taskContext.onFailure(e));
208209
} finally {
@@ -212,10 +213,9 @@ public ClusterState execute(BatchExecutionContext<LazyRolloverTask> batchExecuti
212213
}
213214

214215
if (state != batchExecutionContext.initialState()) {
215-
var reason = new StringBuilder();
216-
Strings.collectionToDelimitedStringWithLimit(results, ",", "lazy bulk rollover [", "]", 1024, reason);
216+
resultsCollector.finish();
217217
try (var ignored = batchExecutionContext.dropHeadersContext()) {
218-
state = allocationService.reroute(state, reason.toString(), listener.reroute());
218+
state = allocationService.reroute(state, reasonBuilder.toString(), listener.reroute());
219219
}
220220
} else {
221221
listener.noRerouteNeeded();
@@ -226,7 +226,7 @@ public ClusterState execute(BatchExecutionContext<LazyRolloverTask> batchExecuti
226226
public ClusterState executeTask(
227227
ProjectState currentState,
228228
RolloverRequest rolloverRequest,
229-
ArrayList<String> results,
229+
Consumer<String> results,
230230
List<TaskContext<LazyRolloverTask>> rolloverTaskContexts,
231231
AllocationActionMultiListener<RolloverResponse> allocationActionMultiListener
232232
) throws Exception {
@@ -263,7 +263,7 @@ public ClusterState executeTask(
263263
null,
264264
isFailureStoreRollover
265265
);
266-
results.add(rolloverResult.sourceIndexName() + "->" + rolloverResult.rolloverIndexName());
266+
results.accept(rolloverResult.sourceIndexName() + "->" + rolloverResult.rolloverIndexName());
267267
logger.trace("lazy rollover result [{}]", rolloverResult);
268268

269269
final var rolloverIndexName = rolloverResult.rolloverIndexName();

‎server/src/main/java/org/elasticsearch/action/admin/indices/rollover/TransportRolloverAction.java‎

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -60,14 +60,14 @@
6060
import org.elasticsearch.transport.TransportService;
6161

6262
import java.time.Instant;
63-
import java.util.ArrayList;
6463
import java.util.Arrays;
6564
import java.util.Collection;
6665
import java.util.HashMap;
6766
import java.util.List;
6867
import java.util.Map;
6968
import java.util.Objects;
7069
import java.util.Optional;
70+
import java.util.function.Consumer;
7171
import java.util.stream.Collectors;
7272

7373
/**
@@ -493,21 +493,21 @@ record RolloverExecutor(
493493
@Override
494494
public ClusterState execute(BatchExecutionContext<RolloverTask> batchExecutionContext) {
495495
final var listener = new AllocationActionMultiListener<RolloverResponse>(threadPool.getThreadContext());
496-
final var results = new ArrayList<String>(batchExecutionContext.taskContexts().size());
496+
final var reasonBuilder = new StringBuilder();
497+
final var resultsCollector = new Strings.BoundedDelimitedStringCollector(reasonBuilder, "bulk rollover [", ",", "]", 1024);
497498
var state = batchExecutionContext.initialState();
498499
for (final var taskContext : batchExecutionContext.taskContexts()) {
499500
try (var ignored = taskContext.captureResponseHeaders()) {
500-
state = executeTask(state, results, taskContext, listener);
501+
state = executeTask(state, resultsCollector::appendItem, taskContext, listener);
501502
} catch (Exception e) {
502503
taskContext.onFailure(e);
503504
}
504505
}
505506

506507
if (state != batchExecutionContext.initialState()) {
507-
var reason = new StringBuilder();
508-
Strings.collectionToDelimitedStringWithLimit(results, ",", "bulk rollover [", "]", 1024, reason);
508+
resultsCollector.finish();
509509
try (var ignored = batchExecutionContext.dropHeadersContext()) {
510-
state = allocationService.reroute(state, reason.toString(), listener.reroute());
510+
state = allocationService.reroute(state, reasonBuilder.toString(), listener.reroute());
511511
}
512512
} else {
513513
listener.noRerouteNeeded();
@@ -517,7 +517,7 @@ public ClusterState execute(BatchExecutionContext<RolloverTask> batchExecutionCo
517517

518518
public ClusterState executeTask(
519519
ClusterState currentState,
520-
ArrayList<String> results,
520+
Consumer<String> resultsCollector,
521521
TaskContext<RolloverTask> rolloverTaskContext,
522522
AllocationActionMultiListener<RolloverResponse> allocationActionMultiListener
523523
) throws Exception {
@@ -588,7 +588,7 @@ public ClusterState executeTask(
588588
rolloverTask.autoShardingResult(),
589589
targetFailureStore
590590
);
591-
results.add(rolloverResult.sourceIndexName() + "->" + rolloverResult.rolloverIndexName());
591+
resultsCollector.accept(rolloverResult.sourceIndexName() + "->" + rolloverResult.rolloverIndexName());
592592
logger.trace("rollover result [{}]", rolloverResult);
593593

594594
final var rolloverIndexName = rolloverResult.rolloverIndexName();

‎server/src/main/java/org/elasticsearch/common/Strings.java‎

Lines changed: 58 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -586,22 +586,65 @@ public static void collectionToDelimitedStringWithLimit(
586586
int appendLimit,
587587
StringBuilder sb
588588
) {
589-
final Iterator<?> it = coll.iterator();
590-
final long lengthLimit = sb.length() + appendLimit; // long to avoid overflow
591-
int count = 0;
592-
while (it.hasNext()) {
593-
sb.append(prefix).append(it.next()).append(suffix);
589+
final var boundedDelimitedStringCollector = new BoundedDelimitedStringCollector(sb, prefix, delim, suffix, appendLimit);
590+
coll.forEach(boundedDelimitedStringCollector::appendItem);
591+
boundedDelimitedStringCollector.finish();
592+
}
593+
594+
/**
595+
* Collects a sequence of objects into a delimited string, dropping objects once the string reaches a certain maximum length. Similar to
596+
* {@link #collectionToDelimitedStringWithLimit} except that this doesn't need the collection of items to be provided up front.
597+
*/
598+
public static final class BoundedDelimitedStringCollector {
599+
private final StringBuilder stringBuilder;
600+
private final String prefix;
601+
private final String delimiter;
602+
private final String suffix;
603+
private final long lengthLimit;
604+
private int count = 0;
605+
private int omitted = 0;
606+
607+
public BoundedDelimitedStringCollector(
608+
StringBuilder stringBuilder,
609+
String prefix,
610+
String delimiter,
611+
String suffix,
612+
int appendLimit
613+
) {
614+
this.stringBuilder = stringBuilder;
615+
this.prefix = prefix;
616+
this.delimiter = delimiter;
617+
this.suffix = suffix;
618+
this.lengthLimit = stringBuilder.length() + appendLimit; // long to avoid overflow
619+
}
620+
621+
/**
622+
* Add the given item's string representation to the string, with a delimiter if necessary and surrounded by the given prefix and
623+
* suffix, as long as the string is not already too long.
624+
*/
625+
public void appendItem(Object item) {
594626
count += 1;
595-
if (it.hasNext()) {
596-
sb.append(delim);
597-
if (sb.length() > lengthLimit) {
598-
int omitted = 0;
599-
while (it.hasNext()) {
600-
it.next();
601-
omitted += 1;
602-
}
603-
sb.append("... (").append(count + omitted).append(" in total, ").append(omitted).append(" omitted)");
604-
}
627+
if (omitted > 0) {
628+
omitted += 1;
629+
return;
630+
}
631+
if (count > 1) {
632+
stringBuilder.append(delimiter);
633+
}
634+
if (stringBuilder.length() > lengthLimit) {
635+
omitted += 1;
636+
stringBuilder.append("..."); // indicate there are some omissions, just in case the caller forgets to call finish()
637+
return;
638+
}
639+
stringBuilder.append(prefix).append(item).append(suffix);
640+
}
641+
642+
/**
643+
* Complete the collection, adding to the string a summary of omitted objects, if any.
644+
*/
645+
public void finish() {
646+
if (omitted > 0) {
647+
stringBuilder.append(" (").append(count).append(" in total, ").append(omitted).append(" omitted)");
605648
}
606649
}
607650
}

‎server/src/main/java/org/elasticsearch/snapshots/SnapshotsService.java‎

Lines changed: 12 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -2506,15 +2506,10 @@ public void onFailure(Exception e) {
25062506
@Override
25072507
public void onFailure(Exception e) {
25082508
logger.warn(() -> {
2509-
final StringBuilder sb = new StringBuilder("failed to complete snapshot deletion for [");
2510-
Strings.collectionToDelimitedStringWithLimit(
2511-
deleteEntry.snapshots().stream().map(SnapshotId::getName).toList(),
2512-
",",
2513-
"",
2514-
"",
2515-
1024,
2516-
sb
2517-
);
2509+
final var sb = new StringBuilder("failed to complete snapshot deletion for [");
2510+
final var collector = new Strings.BoundedDelimitedStringCollector(sb, "", ",", "", 1024);
2511+
deleteEntry.snapshots().forEach(s -> collector.appendItem(s.getName()));
2512+
collector.finish();
25182513
sb.append("] from repository [").append(deleteEntry.repository()).append("]");
25192514
return sb;
25202515
}, e);
@@ -2529,7 +2524,14 @@ protected void handleListeners(List<ActionListener<Void>> deleteListeners) {
25292524
);
25302525
}
25312526
}, () -> {
2532-
logger.info("snapshots {} deleted", snapshotIds);
2527+
logger.info(() -> {
2528+
final var sb = new StringBuilder("snapshots [");
2529+
final var collector = new Strings.BoundedDelimitedStringCollector(sb, "", ",", "", 1024);
2530+
snapshotIds.forEach(collector::appendItem);
2531+
collector.finish();
2532+
sb.append("] deleted");
2533+
return sb;
2534+
});
25332535
doneFuture.onResponse(null);
25342536
});
25352537
}

‎server/src/test/java/org/elasticsearch/common/BoundedDelimitedStringCollectorTests.java‎

Lines changed: 140 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,140 @@
1+
/*
2+
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
3+
* or more contributor license agreements. Licensed under the "Elastic License
4+
* 2.0", the "GNU Affero General Public License v3.0 only", and the "Server Side
5+
* Public License v 1"; you may not use this file except in compliance with, at
6+
* your election, the "Elastic License 2.0", the "GNU Affero General Public
7+
* License v3.0 only", or the "Server Side Public License, v 1".
8+
*/
9+
10+
package org.elasticsearch.common;
11+
12+
import com.carrotsearch.randomizedtesting.annotations.Name;
13+
import com.carrotsearch.randomizedtesting.annotations.ParametersFactory;
14+
15+
import org.elasticsearch.test.ESTestCase;
16+
17+
import java.util.ArrayList;
18+
import java.util.List;
19+
import java.util.stream.Stream;
20+
21+
import static org.elasticsearch.common.Strings.collectionToDelimitedString;
22+
import static org.hamcrest.Matchers.allOf;
23+
import static org.hamcrest.Matchers.containsString;
24+
import static org.hamcrest.Matchers.endsWith;
25+
import static org.hamcrest.Matchers.equalTo;
26+
import static org.hamcrest.Matchers.lessThanOrEqualTo;
27+
28+
public class BoundedDelimitedStringCollectorTests extends ESTestCase {
29+
30+
private interface TestHarness {
31+
String getResult(Iterable<?> collection, String prefix, String delimiter, String suffix, int appendLimit);
32+
33+
enum Type {
34+
COLLECTING,
35+
ITERATING
36+
}
37+
}
38+
39+
private final TestHarness testHarness;
40+
41+
@ParametersFactory
42+
public static Iterable<Object[]> parameters() throws Exception {
43+
return Stream.of(TestHarness.Type.values()).map(x -> new Object[] { x })::iterator;
44+
}
45+
46+
public BoundedDelimitedStringCollectorTests(@Name("type") TestHarness.Type testHarnessType) {
47+
testHarness = switch (testHarnessType) {
48+
case COLLECTING -> (collection, prefix, delimiter, suffix, appendLimit) -> {
49+
final var stringBuilder = new StringBuilder();
50+
final var collector = new Strings.BoundedDelimitedStringCollector(stringBuilder, prefix, delimiter, suffix, appendLimit);
51+
collection.forEach(collector::appendItem);
52+
collector.finish();
53+
return stringBuilder.toString();
54+
};
55+
case ITERATING -> (collection, prefix, delimiter, suffix, appendLimit) -> {
56+
final var stringBuilder = new StringBuilder();
57+
Strings.collectionToDelimitedStringWithLimit(collection, delimiter, prefix, suffix, appendLimit, stringBuilder);
58+
return stringBuilder.toString();
59+
};
60+
};
61+
}
62+
63+
public void testCollectionToDelimitedStringWithLimitZero() {
64+
final String delimiter = randomFrom("", ",", ", ", "/");
65+
final String prefix = randomFrom("", "[");
66+
final String suffix = randomFrom("", "]");
67+
68+
final int count = between(0, 100);
69+
final List<String> strings = new ArrayList<>(count);
70+
while (strings.size() < count) {
71+
// avoid starting with a sequence of empty appends, it makes the assertions much messier
72+
final int minLength = strings.isEmpty() && delimiter.isEmpty() && prefix.isEmpty() && suffix.isEmpty() ? 1 : 0;
73+
strings.add(randomAlphaOfLength(between(minLength, 10)));
74+
}
75+
76+
final String completelyTruncatedDescription = testHarness.getResult(strings, prefix, delimiter, suffix, 0);
77+
78+
if (count == 0) {
79+
assertThat(completelyTruncatedDescription, equalTo(""));
80+
} else if (count == 1) {
81+
assertThat(completelyTruncatedDescription, equalTo(prefix + strings.get(0) + suffix));
82+
} else {
83+
assertThat(
84+
completelyTruncatedDescription,
85+
equalTo(prefix + strings.get(0) + suffix + delimiter + "... (" + count + " in total, " + (count - 1) + " omitted)")
86+
);
87+
}
88+
}
89+
90+
public void testCollectionToDelimitedStringWithLimitTruncation() {
91+
final String delimiter = randomFrom("", ",", ", ", "/");
92+
final String prefix = randomFrom("", "[");
93+
final String suffix = randomFrom("", "]");
94+
95+
final int count = between(2, 100);
96+
final List<String> strings = new ArrayList<>(count);
97+
while (strings.size() < count) {
98+
// avoid empty appends, it makes the assertions much messier
99+
final int minLength = delimiter.isEmpty() && prefix.isEmpty() && suffix.isEmpty() ? 1 : 0;
100+
strings.add(randomAlphaOfLength(between(minLength, 10)));
101+
}
102+
103+
final int fullDescriptionLength = collectionToDelimitedString(strings, delimiter, prefix, suffix).length();
104+
final int lastItemSize = prefix.length() + strings.get(count - 1).length() + suffix.length();
105+
final int truncatedLength = between(0, fullDescriptionLength - lastItemSize - 1);
106+
final String truncatedDescription = testHarness.getResult(strings, prefix, delimiter, suffix, truncatedLength);
107+
108+
assertThat(truncatedDescription, allOf(containsString("... (" + count + " in total,"), endsWith(" omitted)")));
109+
110+
assertThat(
111+
truncatedDescription,
112+
truncatedDescription.length(),
113+
lessThanOrEqualTo(truncatedLength + (prefix + "0123456789" + suffix + delimiter + "... (999 in total, 999 omitted)").length())
114+
);
115+
}
116+
117+
public void testCollectionToDelimitedStringWithLimitNoTruncation() {
118+
final String delimiter = randomFrom("", ",", ", ", "/");
119+
final String prefix = randomFrom("", "[");
120+
final String suffix = randomFrom("", "]");
121+
122+
final int count = between(1, 100);
123+
final List<String> strings = new ArrayList<>(count);
124+
while (strings.size() < count) {
125+
strings.add(randomAlphaOfLength(between(0, 10)));
126+
}
127+
128+
final String fullDescription = collectionToDelimitedString(strings, delimiter, prefix, suffix);
129+
for (String string : strings) {
130+
assertThat(fullDescription, containsString(prefix + string + suffix));
131+
}
132+
133+
final int lastItemSize = prefix.length() + strings.get(count - 1).length() + suffix.length();
134+
final int minLimit = fullDescription.length() - lastItemSize;
135+
final int limit = randomFrom(between(minLimit, fullDescription.length()), between(minLimit, Integer.MAX_VALUE), Integer.MAX_VALUE);
136+
137+
assertThat(testHarness.getResult(strings, prefix, delimiter, suffix, limit), equalTo(fullDescription));
138+
}
139+
140+
}

0 commit comments

Comments
 (0)