Skip to content

Commit f348881

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. Backport of elastic#124303 to `8.x`
1 parent 6e10b86 commit f348881

File tree

6 files changed

+211
-123
lines changed

6 files changed

+211
-123
lines changed

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

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -185,7 +185,8 @@ record LazyRolloverExecutor(
185185
@Override
186186
public ClusterState execute(BatchExecutionContext<LazyRolloverTask> batchExecutionContext) {
187187
final var listener = new AllocationActionMultiListener<RolloverResponse>(threadPool.getThreadContext());
188-
final var results = new ArrayList<String>(batchExecutionContext.taskContexts().size());
188+
var reasonBuilder = new StringBuilder("lazy bulk rollover [");
189+
final var resultsCollector = new Strings.BoundedDelimitedStringCollector(reasonBuilder, ",", 1024);
189190
var state = batchExecutionContext.initialState();
190191
Map<RolloverRequest, List<TaskContext<LazyRolloverTask>>> groupedRequests = new HashMap<>();
191192
for (final var taskContext : batchExecutionContext.taskContexts()) {
@@ -195,7 +196,7 @@ public ClusterState execute(BatchExecutionContext<LazyRolloverTask> batchExecuti
195196
List<TaskContext<LazyRolloverTask>> rolloverTaskContexts = entry.getValue();
196197
try {
197198
RolloverRequest rolloverRequest = entry.getKey();
198-
state = executeTask(state, rolloverRequest, results, rolloverTaskContexts, listener);
199+
state = executeTask(state, rolloverRequest, resultsCollector::appendItem, rolloverTaskContexts, listener);
199200
} catch (Exception e) {
200201
rolloverTaskContexts.forEach(taskContext -> taskContext.onFailure(e));
201202
} finally {
@@ -204,11 +205,10 @@ public ClusterState execute(BatchExecutionContext<LazyRolloverTask> batchExecuti
204205
}
205206

206207
if (state != batchExecutionContext.initialState()) {
207-
var reason = new StringBuilder("lazy bulk rollover [");
208-
Strings.collectionToDelimitedStringWithLimit(results, ",", 1024, reason);
209-
reason.append(']');
208+
resultsCollector.finish();
209+
reasonBuilder.append(']');
210210
try (var ignored = batchExecutionContext.dropHeadersContext()) {
211-
state = allocationService.reroute(state, reason.toString(), listener.reroute());
211+
state = allocationService.reroute(state, reasonBuilder.toString(), listener.reroute());
212212
}
213213
} else {
214214
listener.noRerouteNeeded();
@@ -219,7 +219,7 @@ public ClusterState execute(BatchExecutionContext<LazyRolloverTask> batchExecuti
219219
public ClusterState executeTask(
220220
ClusterState currentState,
221221
RolloverRequest rolloverRequest,
222-
ArrayList<String> results,
222+
Consumer<String> results,
223223
List<TaskContext<LazyRolloverTask>> rolloverTaskContexts,
224224
AllocationActionMultiListener<RolloverResponse> allocationActionMultiListener
225225
) throws Exception {
@@ -256,7 +256,7 @@ public ClusterState executeTask(
256256
null,
257257
isFailureStoreRollover
258258
);
259-
results.add(rolloverResult.sourceIndexName() + "->" + rolloverResult.rolloverIndexName());
259+
results.accept(rolloverResult.sourceIndexName() + "->" + rolloverResult.rolloverIndexName());
260260
logger.trace("lazy rollover result [{}]", rolloverResult);
261261

262262
final var rolloverIndexName = rolloverResult.rolloverIndexName();

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

Lines changed: 9 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -58,14 +58,14 @@
5858
import org.elasticsearch.transport.TransportService;
5959

6060
import java.time.Instant;
61-
import java.util.ArrayList;
6261
import java.util.Arrays;
6362
import java.util.Collection;
6463
import java.util.HashMap;
6564
import java.util.List;
6665
import java.util.Map;
6766
import java.util.Objects;
6867
import java.util.Optional;
68+
import java.util.function.Consumer;
6969
import java.util.stream.Collectors;
7070

7171
/**
@@ -506,22 +506,22 @@ record RolloverExecutor(
506506
@Override
507507
public ClusterState execute(BatchExecutionContext<RolloverTask> batchExecutionContext) {
508508
final var listener = new AllocationActionMultiListener<RolloverResponse>(threadPool.getThreadContext());
509-
final var results = new ArrayList<String>(batchExecutionContext.taskContexts().size());
509+
final var reasonBuilder = new StringBuilder("bulk rollover [");
510+
final var resultsCollector = new Strings.BoundedDelimitedStringCollector(reasonBuilder, ",", 1024);
510511
var state = batchExecutionContext.initialState();
511512
for (final var taskContext : batchExecutionContext.taskContexts()) {
512513
try (var ignored = taskContext.captureResponseHeaders()) {
513-
state = executeTask(state, results, taskContext, listener);
514+
state = executeTask(state, resultsCollector::appendItem, taskContext, listener);
514515
} catch (Exception e) {
515516
taskContext.onFailure(e);
516517
}
517518
}
518519

519520
if (state != batchExecutionContext.initialState()) {
520-
var reason = new StringBuilder("bulk rollover [");
521-
Strings.collectionToDelimitedStringWithLimit(results, ",", 1024, reason);
522-
reason.append(']');
521+
resultsCollector.finish();
522+
reasonBuilder.append(']');
523523
try (var ignored = batchExecutionContext.dropHeadersContext()) {
524-
state = allocationService.reroute(state, reason.toString(), listener.reroute());
524+
state = allocationService.reroute(state, reasonBuilder.toString(), listener.reroute());
525525
}
526526
} else {
527527
listener.noRerouteNeeded();
@@ -531,7 +531,7 @@ public ClusterState execute(BatchExecutionContext<RolloverTask> batchExecutionCo
531531

532532
public ClusterState executeTask(
533533
ClusterState currentState,
534-
ArrayList<String> results,
534+
Consumer<String> resultsCollector,
535535
TaskContext<RolloverTask> rolloverTaskContext,
536536
AllocationActionMultiListener<RolloverResponse> allocationActionMultiListener
537537
) throws Exception {
@@ -603,7 +603,7 @@ public ClusterState executeTask(
603603
rolloverTask.autoShardingResult(),
604604
targetFailureStore
605605
);
606-
results.add(rolloverResult.sourceIndexName() + "->" + rolloverResult.rolloverIndexName());
606+
resultsCollector.accept(rolloverResult.sourceIndexName() + "->" + rolloverResult.rolloverIndexName());
607607
logger.trace("rollover result [{}]", rolloverResult);
608608

609609
final var rolloverIndexName = rolloverResult.rolloverIndexName();

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

Lines changed: 48 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -571,22 +571,55 @@ public static void collectionToDelimitedString(Iterable<?> coll, String delimite
571571
* items are omitted
572572
*/
573573
public static void collectionToDelimitedStringWithLimit(Iterable<?> coll, String delimiter, int appendLimit, StringBuilder sb) {
574-
final Iterator<?> it = coll.iterator();
575-
final long lengthLimit = sb.length() + appendLimit; // long to avoid overflow
576-
int count = 0;
577-
while (it.hasNext()) {
578-
sb.append(it.next());
574+
final var boundedDelimitedStringCollector = new BoundedDelimitedStringCollector(sb, delimiter, appendLimit);
575+
coll.forEach(boundedDelimitedStringCollector::appendItem);
576+
boundedDelimitedStringCollector.finish();
577+
}
578+
579+
/**
580+
* Collects a sequence of objects into a delimited string, dropping objects once the string reaches a certain maximum length. Similar to
581+
* {@link #collectionToDelimitedStringWithLimit} except that this doesn't need the collection of items to be provided up front.
582+
*/
583+
public static final class BoundedDelimitedStringCollector {
584+
private final StringBuilder stringBuilder;
585+
private final String delimiter;
586+
private final long lengthLimit;
587+
private int count = 0;
588+
private int omitted = 0;
589+
590+
public BoundedDelimitedStringCollector(StringBuilder stringBuilder, String delimiter, int appendLimit) {
591+
this.stringBuilder = stringBuilder;
592+
this.delimiter = delimiter;
593+
this.lengthLimit = stringBuilder.length() + appendLimit; // long to avoid overflow
594+
}
595+
596+
/**
597+
* Add the given item's string representation to the string, with a delimiter if necessary and surrounded by the given prefix and
598+
* suffix, as long as the string is not already too long.
599+
*/
600+
public void appendItem(Object item) {
579601
count += 1;
580-
if (it.hasNext()) {
581-
sb.append(delimiter);
582-
if (sb.length() > lengthLimit) {
583-
int omitted = 0;
584-
while (it.hasNext()) {
585-
it.next();
586-
omitted += 1;
587-
}
588-
sb.append("... (").append(count + omitted).append(" in total, ").append(omitted).append(" omitted)");
589-
}
602+
if (omitted > 0) {
603+
omitted += 1;
604+
return;
605+
}
606+
if (count > 1) {
607+
stringBuilder.append(delimiter);
608+
}
609+
if (stringBuilder.length() > lengthLimit) {
610+
omitted += 1;
611+
stringBuilder.append("..."); // indicate there are some omissions, just in case the caller forgets to call finish()
612+
return;
613+
}
614+
stringBuilder.append(item);
615+
}
616+
617+
/**
618+
* Complete the collection, adding to the string a summary of omitted objects, if any.
619+
*/
620+
public void finish() {
621+
if (omitted > 0) {
622+
stringBuilder.append(" (").append(count).append(" in total, ").append(omitted).append(" omitted)");
590623
}
591624
}
592625
}

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

Lines changed: 12 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -2507,13 +2507,10 @@ public void onFailure(Exception e) {
25072507
@Override
25082508
public void onFailure(Exception e) {
25092509
logger.warn(() -> {
2510-
final StringBuilder sb = new StringBuilder("failed to complete snapshot deletion for [");
2511-
Strings.collectionToDelimitedStringWithLimit(
2512-
deleteEntry.snapshots().stream().map(SnapshotId::getName).toList(),
2513-
",",
2514-
1024,
2515-
sb
2516-
);
2510+
final var sb = new StringBuilder("failed to complete snapshot deletion for [");
2511+
final var collector = new Strings.BoundedDelimitedStringCollector(sb, ",", 1024);
2512+
deleteEntry.snapshots().forEach(s -> collector.appendItem(s.getName()));
2513+
collector.finish();
25172514
sb.append("] from repository [").append(deleteEntry.repository()).append("]");
25182515
return sb;
25192516
}, e);
@@ -2528,7 +2525,14 @@ protected void handleListeners(List<ActionListener<Void>> deleteListeners) {
25282525
);
25292526
}
25302527
}, () -> {
2531-
logger.info("snapshots {} deleted", snapshotIds);
2528+
logger.info(() -> {
2529+
final var sb = new StringBuilder("snapshots [");
2530+
final var collector = new Strings.BoundedDelimitedStringCollector(sb, ",", 1024);
2531+
snapshotIds.forEach(collector::appendItem);
2532+
collector.finish();
2533+
sb.append("] deleted");
2534+
return sb;
2535+
});
25322536
doneFuture.onResponse(null);
25332537
});
25342538
}

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

Lines changed: 134 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,134 @@
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 delimiter, 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, delimiter, appendLimit) -> {
49+
final var stringBuilder = new StringBuilder();
50+
final var collector = new Strings.BoundedDelimitedStringCollector(stringBuilder, delimiter, appendLimit);
51+
collection.forEach(collector::appendItem);
52+
collector.finish();
53+
return stringBuilder.toString();
54+
};
55+
case ITERATING -> (collection, delimiter, appendLimit) -> {
56+
final var stringBuilder = new StringBuilder();
57+
Strings.collectionToDelimitedStringWithLimit(collection, delimiter, appendLimit, stringBuilder);
58+
return stringBuilder.toString();
59+
};
60+
};
61+
}
62+
63+
public void testCollectionToDelimitedStringWithLimitZero() {
64+
final String delimiter = randomFrom("", ",", ", ", "/");
65+
66+
final int count = between(0, 100);
67+
final List<String> strings = new ArrayList<>(count);
68+
while (strings.size() < count) {
69+
// avoid starting with a sequence of empty appends, it makes the assertions much messier
70+
final int minLength = strings.isEmpty() && delimiter.isEmpty() ? 1 : 0;
71+
strings.add(randomAlphaOfLength(between(minLength, 10)));
72+
}
73+
74+
final String completelyTruncatedDescription = testHarness.getResult(strings, delimiter, 0);
75+
76+
if (count == 0) {
77+
assertThat(completelyTruncatedDescription, equalTo(""));
78+
} else if (count == 1) {
79+
assertThat(completelyTruncatedDescription, equalTo(strings.get(0)));
80+
} else {
81+
assertThat(
82+
completelyTruncatedDescription,
83+
equalTo(strings.get(0) + delimiter + "... (" + count + " in total, " + (count - 1) + " omitted)")
84+
);
85+
}
86+
}
87+
88+
public void testCollectionToDelimitedStringWithLimitTruncation() {
89+
final String delimiter = randomFrom("", ",", ", ", "/");
90+
91+
final int count = between(2, 100);
92+
final List<String> strings = new ArrayList<>(count);
93+
while (strings.size() < count) {
94+
// avoid empty appends, it makes the assertions much messier
95+
final int minLength = delimiter.isEmpty() ? 1 : 0;
96+
strings.add(randomAlphaOfLength(between(minLength, 10)));
97+
}
98+
99+
final int fullDescriptionLength = collectionToDelimitedString(strings, delimiter).length();
100+
final int lastItemSize = strings.get(count - 1).length();
101+
final int truncatedLength = between(0, fullDescriptionLength - lastItemSize - 1);
102+
final String truncatedDescription = testHarness.getResult(strings, delimiter, truncatedLength);
103+
104+
assertThat(truncatedDescription, allOf(containsString("... (" + count + " in total,"), endsWith(" omitted)")));
105+
106+
assertThat(
107+
truncatedDescription,
108+
truncatedDescription.length(),
109+
lessThanOrEqualTo(truncatedLength + ("0123456789" + delimiter + "... (999 in total, 999 omitted)").length())
110+
);
111+
}
112+
113+
public void testCollectionToDelimitedStringWithLimitNoTruncation() {
114+
final String delimiter = randomFrom("", ",", ", ", "/");
115+
116+
final int count = between(1, 100);
117+
final List<String> strings = new ArrayList<>(count);
118+
while (strings.size() < count) {
119+
strings.add(randomAlphaOfLength(between(0, 10)));
120+
}
121+
122+
final String fullDescription = collectionToDelimitedString(strings, delimiter);
123+
for (String string : strings) {
124+
assertThat(fullDescription, containsString(string));
125+
}
126+
127+
final int lastItemSize = strings.get(count - 1).length();
128+
final int minLimit = fullDescription.length() - lastItemSize;
129+
final int limit = randomFrom(between(minLimit, fullDescription.length()), between(minLimit, Integer.MAX_VALUE), Integer.MAX_VALUE);
130+
131+
assertThat(testHarness.getResult(strings, delimiter, limit), equalTo(fullDescription));
132+
}
133+
134+
}

0 commit comments

Comments
 (0)