Introduce BoundedDelimitedStringCollector

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

@@ -188,7 +188,8 @@ record LazyRolloverExecutor(
         @Override
         public ClusterState execute(BatchExecutionContext<LazyRolloverTask> batchExecutionContext) {
             final var listener = new AllocationActionMultiListener<RolloverResponse>(threadPool.getThreadContext());
-            final var results = new ArrayList<String>(batchExecutionContext.taskContexts().size());
+            final var reasonBuilder = new StringBuilder();
+            final var resultsCollector = new Strings.BoundedDelimitedStringCollector(reasonBuilder, "lazy bulk rollover [", ",", "]", 1024);
             var state = batchExecutionContext.initialState();
             Map<ProjectId, Map<RolloverRequest, List<TaskContext<LazyRolloverTask>>>> groupedRequests = new HashMap<>();
             for (final var taskContext : batchExecutionContext.taskContexts()) {
@@ -202,7 +203,7 @@ public ClusterState execute(BatchExecutionContext<LazyRolloverTask> batchExecuti
                     try {
                         RolloverRequest rolloverRequest = entry.getKey();
                         final var projectState = state.projectState(projectRequests.getKey());
-                        state = executeTask(projectState, rolloverRequest, results, rolloverTaskContexts, listener);
+                        state = executeTask(projectState, rolloverRequest, resultsCollector::appendItem, rolloverTaskContexts, listener);
                     } catch (Exception e) {
                         rolloverTaskContexts.forEach(taskContext -> taskContext.onFailure(e));
                     } finally {
@@ -212,10 +213,9 @@ public ClusterState execute(BatchExecutionContext<LazyRolloverTask> batchExecuti
             }
 
             if (state != batchExecutionContext.initialState()) {
-                var reason = new StringBuilder();
-                Strings.collectionToDelimitedStringWithLimit(results, ",", "lazy bulk rollover [", "]", 1024, reason);
+                resultsCollector.finish();
                 try (var ignored = batchExecutionContext.dropHeadersContext()) {
-                    state = allocationService.reroute(state, reason.toString(), listener.reroute());
+                    state = allocationService.reroute(state, reasonBuilder.toString(), listener.reroute());
                 }
             } else {
                 listener.noRerouteNeeded();
@@ -226,7 +226,7 @@ public ClusterState execute(BatchExecutionContext<LazyRolloverTask> batchExecuti
         public ClusterState executeTask(
             ProjectState currentState,
             RolloverRequest rolloverRequest,
-            ArrayList<String> results,
+            Consumer<String> results,
             List<TaskContext<LazyRolloverTask>> rolloverTaskContexts,
             AllocationActionMultiListener<RolloverResponse> allocationActionMultiListener
         ) throws Exception {
@@ -263,7 +263,7 @@ public ClusterState executeTask(
                 null,
                 isFailureStoreRollover
             );
-            results.add(rolloverResult.sourceIndexName() + "->" + rolloverResult.rolloverIndexName());
+            results.accept(rolloverResult.sourceIndexName() + "->" + rolloverResult.rolloverIndexName());
             logger.trace("lazy rollover result [{}]", rolloverResult);
 
             final var rolloverIndexName = rolloverResult.rolloverIndexName();

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

@@ -60,14 +60,14 @@
 import org.elasticsearch.transport.TransportService;
 
 import java.time.Instant;
-import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Collection;
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
 import java.util.Objects;
 import java.util.Optional;
+import java.util.function.Consumer;
 import java.util.stream.Collectors;
 
 /**
@@ -493,21 +493,21 @@ record RolloverExecutor(
         @Override
         public ClusterState execute(BatchExecutionContext<RolloverTask> batchExecutionContext) {
             final var listener = new AllocationActionMultiListener<RolloverResponse>(threadPool.getThreadContext());
-            final var results = new ArrayList<String>(batchExecutionContext.taskContexts().size());
+            final var reasonBuilder = new StringBuilder();
+            final var resultsCollector = new Strings.BoundedDelimitedStringCollector(reasonBuilder, "bulk rollover [", ",", "]", 1024);
             var state = batchExecutionContext.initialState();
             for (final var taskContext : batchExecutionContext.taskContexts()) {
                 try (var ignored = taskContext.captureResponseHeaders()) {
-                    state = executeTask(state, results, taskContext, listener);
+                    state = executeTask(state, resultsCollector::appendItem, taskContext, listener);
                 } catch (Exception e) {
                     taskContext.onFailure(e);
                 }
             }
 
             if (state != batchExecutionContext.initialState()) {
-                var reason = new StringBuilder();
-                Strings.collectionToDelimitedStringWithLimit(results, ",", "bulk rollover [", "]", 1024, reason);
+                resultsCollector.finish();
                 try (var ignored = batchExecutionContext.dropHeadersContext()) {
-                    state = allocationService.reroute(state, reason.toString(), listener.reroute());
+                    state = allocationService.reroute(state, reasonBuilder.toString(), listener.reroute());
                 }
             } else {
                 listener.noRerouteNeeded();
@@ -517,7 +517,7 @@ public ClusterState execute(BatchExecutionContext<RolloverTask> batchExecutionCo
 
         public ClusterState executeTask(
             ClusterState currentState,
-            ArrayList<String> results,
+            Consumer<String> resultsCollector,
             TaskContext<RolloverTask> rolloverTaskContext,
             AllocationActionMultiListener<RolloverResponse> allocationActionMultiListener
         ) throws Exception {
@@ -588,7 +588,7 @@ public ClusterState executeTask(
                     rolloverTask.autoShardingResult(),
                     targetFailureStore
                 );
-                results.add(rolloverResult.sourceIndexName() + "->" + rolloverResult.rolloverIndexName());
+                resultsCollector.accept(rolloverResult.sourceIndexName() + "->" + rolloverResult.rolloverIndexName());
                 logger.trace("rollover result [{}]", rolloverResult);
 
                 final var rolloverIndexName = rolloverResult.rolloverIndexName();

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

@@ -586,22 +586,65 @@ public static void collectionToDelimitedStringWithLimit(
         int appendLimit,
         StringBuilder sb
     ) {
-        final Iterator<?> it = coll.iterator();
-        final long lengthLimit = sb.length() + appendLimit; // long to avoid overflow
-        int count = 0;
-        while (it.hasNext()) {
-            sb.append(prefix).append(it.next()).append(suffix);
+        final var boundedDelimitedStringCollector = new BoundedDelimitedStringCollector(sb, prefix, delim, suffix, appendLimit);
+        coll.forEach(boundedDelimitedStringCollector::appendItem);
+        boundedDelimitedStringCollector.finish();
+    }
+
+    /**
+     * Collects a sequence of objects into a delimited string, dropping objects once the string reaches a certain maximum length. Similar to
+     * {@link #collectionToDelimitedStringWithLimit} except that this doesn't need the collection of items to be provided up front.
+     */
+    public static final class BoundedDelimitedStringCollector {
+        private final StringBuilder stringBuilder;
+        private final String prefix;
+        private final String delimiter;
+        private final String suffix;
+        private final long lengthLimit;
+        private int count = 0;
+        private int omitted = 0;
+
+        public BoundedDelimitedStringCollector(
+            StringBuilder stringBuilder,
+            String prefix,
+            String delimiter,
+            String suffix,
+            int appendLimit
+        ) {
+            this.stringBuilder = stringBuilder;
+            this.prefix = prefix;
+            this.delimiter = delimiter;
+            this.suffix = suffix;
+            this.lengthLimit = stringBuilder.length() + appendLimit; // long to avoid overflow
+        }
+
+        /**
+         * Add the given item's string representation to the string, with a delimiter if necessary and surrounded by the given prefix and
+         * suffix, as long as the string is not already too long.
+         */
+        public void appendItem(Object item) {
             count += 1;
-            if (it.hasNext()) {
-                sb.append(delim);
-                if (sb.length() > lengthLimit) {
-                    int omitted = 0;
-                    while (it.hasNext()) {
-                        it.next();
-                        omitted += 1;
-                    }
-                    sb.append("... (").append(count + omitted).append(" in total, ").append(omitted).append(" omitted)");
-                }
+            if (omitted > 0) {
+                omitted += 1;
+                return;
+            }
+            if (count > 1) {
+                stringBuilder.append(delimiter);
+            }
+            if (stringBuilder.length() > lengthLimit) {
+                omitted += 1;
+                stringBuilder.append("..."); // indicate there are some omissions, just in case the caller forgets to call finish()
+                return;
+            }
+            stringBuilder.append(prefix).append(item).append(suffix);
+        }
+
+        /**
+         * Complete the collection, adding to the string a summary of omitted objects, if any.
+         */
+        public void finish() {
+            if (omitted > 0) {
+                stringBuilder.append(" (").append(count).append(" in total, ").append(omitted).append(" omitted)");
             }
         }
     }

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

@@ -2506,15 +2506,10 @@ public void onFailure(Exception e) {
                     @Override
                     public void onFailure(Exception e) {
                         logger.warn(() -> {
-                            final StringBuilder sb = new StringBuilder("failed to complete snapshot deletion for [");
-                            Strings.collectionToDelimitedStringWithLimit(
-                                deleteEntry.snapshots().stream().map(SnapshotId::getName).toList(),
-                                ",",
-                                "",
-                                "",
-                                1024,
-                                sb
-                            );
+                            final var sb = new StringBuilder("failed to complete snapshot deletion for [");
+                            final var collector = new Strings.BoundedDelimitedStringCollector(sb, "", ",", "", 1024);
+                            deleteEntry.snapshots().forEach(s -> collector.appendItem(s.getName()));
+                            collector.finish();
                             sb.append("] from repository [").append(deleteEntry.repository()).append("]");
                             return sb;
                         }, e);
@@ -2529,7 +2524,14 @@ protected void handleListeners(List<ActionListener<Void>> deleteListeners) {
                         );
                     }
                 }, () -> {
-                    logger.info("snapshots {} deleted", snapshotIds);
+                    logger.info(() -> {
+                        final var sb = new StringBuilder("snapshots [");
+                        final var collector = new Strings.BoundedDelimitedStringCollector(sb, "", ",", "", 1024);
+                        snapshotIds.forEach(collector::appendItem);
+                        collector.finish();
+                        sb.append("] deleted");
+                        return sb;
+                    });
                     doneFuture.onResponse(null);
                 });
         }

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

@@ -0,0 +1,140 @@
+/*
+ * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
+ * or more contributor license agreements. Licensed under the "Elastic License
+ * 2.0", the "GNU Affero General Public License v3.0 only", and the "Server Side
+ * Public License v 1"; you may not use this file except in compliance with, at
+ * your election, the "Elastic License 2.0", the "GNU Affero General Public
+ * License v3.0 only", or the "Server Side Public License, v 1".
+ */
+
+package org.elasticsearch.common;
+
+import com.carrotsearch.randomizedtesting.annotations.Name;
+import com.carrotsearch.randomizedtesting.annotations.ParametersFactory;
+
+import org.elasticsearch.test.ESTestCase;
+
+import java.util.ArrayList;
+import java.util.List;
+import java.util.stream.Stream;
+
+import static org.elasticsearch.common.Strings.collectionToDelimitedString;
+import static org.hamcrest.Matchers.allOf;
+import static org.hamcrest.Matchers.containsString;
+import static org.hamcrest.Matchers.endsWith;
+import static org.hamcrest.Matchers.equalTo;
+import static org.hamcrest.Matchers.lessThanOrEqualTo;
+
+public class BoundedDelimitedStringCollectorTests extends ESTestCase {
+
+    private interface TestHarness {
+        String getResult(Iterable<?> collection, String prefix, String delimiter, String suffix, int appendLimit);
+
+        enum Type {
+            COLLECTING,
+            ITERATING
+        }
+    }
+
+    private final TestHarness testHarness;
+
+    @ParametersFactory
+    public static Iterable<Object[]> parameters() throws Exception {
+        return Stream.of(TestHarness.Type.values()).map(x -> new Object[] { x })::iterator;
+    }
+
+    public BoundedDelimitedStringCollectorTests(@Name("type") TestHarness.Type testHarnessType) {
+        testHarness = switch (testHarnessType) {
+            case COLLECTING -> (collection, prefix, delimiter, suffix, appendLimit) -> {
+                final var stringBuilder = new StringBuilder();
+                final var collector = new Strings.BoundedDelimitedStringCollector(stringBuilder, prefix, delimiter, suffix, appendLimit);
+                collection.forEach(collector::appendItem);
+                collector.finish();
+                return stringBuilder.toString();
+            };
+            case ITERATING -> (collection, prefix, delimiter, suffix, appendLimit) -> {
+                final var stringBuilder = new StringBuilder();
+                Strings.collectionToDelimitedStringWithLimit(collection, delimiter, prefix, suffix, appendLimit, stringBuilder);
+                return stringBuilder.toString();
+            };
+        };
+    }
+
+    public void testCollectionToDelimitedStringWithLimitZero() {
+        final String delimiter = randomFrom("", ",", ", ", "/");
+        final String prefix = randomFrom("", "[");
+        final String suffix = randomFrom("", "]");
+
+        final int count = between(0, 100);
+        final List<String> strings = new ArrayList<>(count);
+        while (strings.size() < count) {
+            // avoid starting with a sequence of empty appends, it makes the assertions much messier
+            final int minLength = strings.isEmpty() && delimiter.isEmpty() && prefix.isEmpty() && suffix.isEmpty() ? 1 : 0;
+            strings.add(randomAlphaOfLength(between(minLength, 10)));
+        }
+
+        final String completelyTruncatedDescription = testHarness.getResult(strings, prefix, delimiter, suffix, 0);
+
+        if (count == 0) {
+            assertThat(completelyTruncatedDescription, equalTo(""));
+        } else if (count == 1) {
+            assertThat(completelyTruncatedDescription, equalTo(prefix + strings.get(0) + suffix));
+        } else {
+            assertThat(
+                completelyTruncatedDescription,
+                equalTo(prefix + strings.get(0) + suffix + delimiter + "... (" + count + " in total, " + (count - 1) + " omitted)")
+            );
+        }
+    }
+
+    public void testCollectionToDelimitedStringWithLimitTruncation() {
+        final String delimiter = randomFrom("", ",", ", ", "/");
+        final String prefix = randomFrom("", "[");
+        final String suffix = randomFrom("", "]");
+
+        final int count = between(2, 100);
+        final List<String> strings = new ArrayList<>(count);
+        while (strings.size() < count) {
+            // avoid empty appends, it makes the assertions much messier
+            final int minLength = delimiter.isEmpty() && prefix.isEmpty() && suffix.isEmpty() ? 1 : 0;
+            strings.add(randomAlphaOfLength(between(minLength, 10)));
+        }
+
+        final int fullDescriptionLength = collectionToDelimitedString(strings, delimiter, prefix, suffix).length();
+        final int lastItemSize = prefix.length() + strings.get(count - 1).length() + suffix.length();
+        final int truncatedLength = between(0, fullDescriptionLength - lastItemSize - 1);
+        final String truncatedDescription = testHarness.getResult(strings, prefix, delimiter, suffix, truncatedLength);
+
+        assertThat(truncatedDescription, allOf(containsString("... (" + count + " in total,"), endsWith(" omitted)")));
+
+        assertThat(
+            truncatedDescription,
+            truncatedDescription.length(),
+            lessThanOrEqualTo(truncatedLength + (prefix + "0123456789" + suffix + delimiter + "... (999 in total, 999 omitted)").length())
+        );
+    }
+
+    public void testCollectionToDelimitedStringWithLimitNoTruncation() {
+        final String delimiter = randomFrom("", ",", ", ", "/");
+        final String prefix = randomFrom("", "[");
+        final String suffix = randomFrom("", "]");
+
+        final int count = between(1, 100);
+        final List<String> strings = new ArrayList<>(count);
+        while (strings.size() < count) {
+            strings.add(randomAlphaOfLength(between(0, 10)));
+        }
+
+        final String fullDescription = collectionToDelimitedString(strings, delimiter, prefix, suffix);
+        for (String string : strings) {
+            assertThat(fullDescription, containsString(prefix + string + suffix));
+        }
+
+        final int lastItemSize = prefix.length() + strings.get(count - 1).length() + suffix.length();
+        final int minLimit = fullDescription.length() - lastItemSize;
+        final int limit = randomFrom(between(minLimit, fullDescription.length()), between(minLimit, Integer.MAX_VALUE), Integer.MAX_VALUE);
+
+        assertThat(testHarness.getResult(strings, prefix, delimiter, suffix, limit), equalTo(fullDescription));
+    }
+
+}