diff --git a/server/src/main/java/org/elasticsearch/action/admin/indices/rollover/LazyRolloverAction.java b/server/src/main/java/org/elasticsearch/action/admin/indices/rollover/LazyRolloverAction.java index 0def598affe32..b7a6d9dfa960a 100644 --- a/server/src/main/java/org/elasticsearch/action/admin/indices/rollover/LazyRolloverAction.java +++ b/server/src/main/java/org/elasticsearch/action/admin/indices/rollover/LazyRolloverAction.java @@ -188,7 +188,8 @@ record LazyRolloverExecutor( @Override public ClusterState execute(BatchExecutionContext batchExecutionContext) { final var listener = new AllocationActionMultiListener(threadPool.getThreadContext()); - final var results = new ArrayList(batchExecutionContext.taskContexts().size()); + var reasonBuilder = new StringBuilder("lazy bulk rollover ["); + final var resultsCollector = new Strings.BoundedDelimitedStringCollector(reasonBuilder, ",", 1024); var state = batchExecutionContext.initialState(); Map>>> groupedRequests = new HashMap<>(); for (final var taskContext : batchExecutionContext.taskContexts()) { @@ -202,7 +203,7 @@ public ClusterState execute(BatchExecutionContext 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,11 +213,10 @@ public ClusterState execute(BatchExecutionContext batchExecuti } if (state != batchExecutionContext.initialState()) { - var reason = new StringBuilder("lazy bulk rollover ["); - Strings.collectionToDelimitedStringWithLimit(results, ",", 1024, reason); - reason.append(']'); + resultsCollector.finish(); + reasonBuilder.append(']'); try (var ignored = batchExecutionContext.dropHeadersContext()) { - state = allocationService.reroute(state, reason.toString(), listener.reroute()); + state = allocationService.reroute(state, reasonBuilder.toString(), listener.reroute()); } } else { listener.noRerouteNeeded(); @@ -227,7 +227,7 @@ public ClusterState execute(BatchExecutionContext batchExecuti public ClusterState executeTask( ProjectState currentState, RolloverRequest rolloverRequest, - ArrayList results, + Consumer results, List> rolloverTaskContexts, AllocationActionMultiListener allocationActionMultiListener ) throws Exception { @@ -264,7 +264,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(); diff --git a/server/src/main/java/org/elasticsearch/action/admin/indices/rollover/TransportRolloverAction.java b/server/src/main/java/org/elasticsearch/action/admin/indices/rollover/TransportRolloverAction.java index b799122d68dfb..432d720cb4e38 100644 --- a/server/src/main/java/org/elasticsearch/action/admin/indices/rollover/TransportRolloverAction.java +++ b/server/src/main/java/org/elasticsearch/action/admin/indices/rollover/TransportRolloverAction.java @@ -60,7 +60,6 @@ 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; @@ -68,6 +67,7 @@ import java.util.Map; import java.util.Objects; import java.util.Optional; +import java.util.function.Consumer; import java.util.stream.Collectors; /** @@ -517,22 +517,22 @@ record RolloverExecutor( @Override public ClusterState execute(BatchExecutionContext batchExecutionContext) { final var listener = new AllocationActionMultiListener(threadPool.getThreadContext()); - final var results = new ArrayList(batchExecutionContext.taskContexts().size()); + final var reasonBuilder = new StringBuilder("bulk rollover ["); + final var resultsCollector = new Strings.BoundedDelimitedStringCollector(reasonBuilder, ",", 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("bulk rollover ["); - Strings.collectionToDelimitedStringWithLimit(results, ",", 1024, reason); - reason.append(']'); + resultsCollector.finish(); + reasonBuilder.append(']'); try (var ignored = batchExecutionContext.dropHeadersContext()) { - state = allocationService.reroute(state, reason.toString(), listener.reroute()); + state = allocationService.reroute(state, reasonBuilder.toString(), listener.reroute()); } } else { listener.noRerouteNeeded(); @@ -542,7 +542,7 @@ public ClusterState execute(BatchExecutionContext batchExecutionCo public ClusterState executeTask( ClusterState currentState, - ArrayList results, + Consumer resultsCollector, TaskContext rolloverTaskContext, AllocationActionMultiListener allocationActionMultiListener ) throws Exception { @@ -613,7 +613,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(); diff --git a/server/src/main/java/org/elasticsearch/common/Strings.java b/server/src/main/java/org/elasticsearch/common/Strings.java index fb655fbbc30a9..b87112386b440 100644 --- a/server/src/main/java/org/elasticsearch/common/Strings.java +++ b/server/src/main/java/org/elasticsearch/common/Strings.java @@ -575,22 +575,55 @@ public static void collectionToDelimitedString(Iterable coll, String delimite * items are omitted */ public static void collectionToDelimitedStringWithLimit(Iterable coll, String delimiter, 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(it.next()); + final var boundedDelimitedStringCollector = new BoundedDelimitedStringCollector(sb, delimiter, 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 delimiter; + private final long lengthLimit; + private int count = 0; + private int omitted = 0; + + public BoundedDelimitedStringCollector(StringBuilder stringBuilder, String delimiter, int appendLimit) { + this.stringBuilder = stringBuilder; + this.delimiter = delimiter; + 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(delimiter); - 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(item); + } + + /** + * 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)"); } } } diff --git a/server/src/main/java/org/elasticsearch/snapshots/SnapshotsService.java b/server/src/main/java/org/elasticsearch/snapshots/SnapshotsService.java index 9c644826fcfec..983e9c36c7bdd 100644 --- a/server/src/main/java/org/elasticsearch/snapshots/SnapshotsService.java +++ b/server/src/main/java/org/elasticsearch/snapshots/SnapshotsService.java @@ -2506,13 +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); @@ -2527,7 +2524,14 @@ protected void handleListeners(List> 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); }); } diff --git a/server/src/test/java/org/elasticsearch/common/BoundedDelimitedStringCollectorTests.java b/server/src/test/java/org/elasticsearch/common/BoundedDelimitedStringCollectorTests.java new file mode 100644 index 0000000000000..7d2ce5fd1d55a --- /dev/null +++ b/server/src/test/java/org/elasticsearch/common/BoundedDelimitedStringCollectorTests.java @@ -0,0 +1,134 @@ +/* + * 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 delimiter, int appendLimit); + + enum Type { + COLLECTING, + ITERATING + } + } + + private final TestHarness testHarness; + + @ParametersFactory + public static Iterable 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, delimiter, appendLimit) -> { + final var stringBuilder = new StringBuilder(); + final var collector = new Strings.BoundedDelimitedStringCollector(stringBuilder, delimiter, appendLimit); + collection.forEach(collector::appendItem); + collector.finish(); + return stringBuilder.toString(); + }; + case ITERATING -> (collection, delimiter, appendLimit) -> { + final var stringBuilder = new StringBuilder(); + Strings.collectionToDelimitedStringWithLimit(collection, delimiter, appendLimit, stringBuilder); + return stringBuilder.toString(); + }; + }; + } + + public void testCollectionToDelimitedStringWithLimitZero() { + final String delimiter = randomFrom("", ",", ", ", "/"); + + final int count = between(0, 100); + final List 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() ? 1 : 0; + strings.add(randomAlphaOfLength(between(minLength, 10))); + } + + final String completelyTruncatedDescription = testHarness.getResult(strings, delimiter, 0); + + if (count == 0) { + assertThat(completelyTruncatedDescription, equalTo("")); + } else if (count == 1) { + assertThat(completelyTruncatedDescription, equalTo(strings.get(0))); + } else { + assertThat( + completelyTruncatedDescription, + equalTo(strings.get(0) + delimiter + "... (" + count + " in total, " + (count - 1) + " omitted)") + ); + } + } + + public void testCollectionToDelimitedStringWithLimitTruncation() { + final String delimiter = randomFrom("", ",", ", ", "/"); + + final int count = between(2, 100); + final List strings = new ArrayList<>(count); + while (strings.size() < count) { + // avoid empty appends, it makes the assertions much messier + final int minLength = delimiter.isEmpty() ? 1 : 0; + strings.add(randomAlphaOfLength(between(minLength, 10))); + } + + final int fullDescriptionLength = collectionToDelimitedString(strings, delimiter).length(); + final int lastItemSize = strings.get(count - 1).length(); + final int truncatedLength = between(0, fullDescriptionLength - lastItemSize - 1); + final String truncatedDescription = testHarness.getResult(strings, delimiter, truncatedLength); + + assertThat(truncatedDescription, allOf(containsString("... (" + count + " in total,"), endsWith(" omitted)"))); + + assertThat( + truncatedDescription, + truncatedDescription.length(), + lessThanOrEqualTo(truncatedLength + ("0123456789" + delimiter + "... (999 in total, 999 omitted)").length()) + ); + } + + public void testCollectionToDelimitedStringWithLimitNoTruncation() { + final String delimiter = randomFrom("", ",", ", ", "/"); + + final int count = between(1, 100); + final List strings = new ArrayList<>(count); + while (strings.size() < count) { + strings.add(randomAlphaOfLength(between(0, 10))); + } + + final String fullDescription = collectionToDelimitedString(strings, delimiter); + for (String string : strings) { + assertThat(fullDescription, containsString(string)); + } + + final int lastItemSize = strings.get(count - 1).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, delimiter, limit), equalTo(fullDescription)); + } + +} diff --git a/server/src/test/java/org/elasticsearch/common/StringsTests.java b/server/src/test/java/org/elasticsearch/common/StringsTests.java index 36ab4438dd487..a543ae73997b3 100644 --- a/server/src/test/java/org/elasticsearch/common/StringsTests.java +++ b/server/src/test/java/org/elasticsearch/common/StringsTests.java @@ -16,7 +16,6 @@ import java.util.ArrayList; import java.util.Collections; -import java.util.List; import java.util.Locale; import java.util.stream.Collectors; import java.util.stream.IntStream; @@ -24,8 +23,6 @@ import static com.carrotsearch.randomizedtesting.RandomizedTest.getRandom; import static org.elasticsearch.common.Strings.INVALID_CHARS; import static org.elasticsearch.common.Strings.cleanTruncate; -import static org.elasticsearch.common.Strings.collectionToDelimitedString; -import static org.elasticsearch.common.Strings.collectionToDelimitedStringWithLimit; import static org.elasticsearch.common.Strings.deleteAny; import static org.elasticsearch.common.Strings.delimitedListToStringArray; import static org.elasticsearch.common.Strings.hasLength; @@ -38,15 +35,12 @@ import static org.elasticsearch.common.Strings.toLowercaseAscii; import static org.elasticsearch.common.Strings.tokenizeByCommaToSet; import static org.elasticsearch.common.Strings.trimLeadingCharacter; -import static org.hamcrest.Matchers.allOf; import static org.hamcrest.Matchers.arrayContaining; import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.emptyArray; -import static org.hamcrest.Matchers.endsWith; import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.hasToString; import static org.hamcrest.Matchers.is; -import static org.hamcrest.Matchers.lessThanOrEqualTo; public class StringsTests extends ESTestCase { @@ -226,83 +220,6 @@ public void testDelimitedListToStringArray() { assertThat(delimitedListToStringArray("abcd,abce,abcdf,bcad,a", ",a", "d"), arrayContaining("abc", "bce", "bcf,bca", "")); } - public void testCollectionToDelimitedStringWithLimitZero() { - final String delimiter = randomFrom("", ",", ", ", "/"); - - final int count = between(0, 100); - final List 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() ? 1 : 0; - strings.add(randomAlphaOfLength(between(minLength, 10))); - } - - final StringBuilder stringBuilder = new StringBuilder(); - collectionToDelimitedStringWithLimit(strings, delimiter, 0, stringBuilder); - final String completelyTruncatedDescription = stringBuilder.toString(); - - if (count == 0) { - assertThat(completelyTruncatedDescription, equalTo("")); - } else if (count == 1) { - assertThat(completelyTruncatedDescription, equalTo(strings.get(0))); - } else { - assertThat( - completelyTruncatedDescription, - equalTo(strings.get(0) + delimiter + "... (" + count + " in total, " + (count - 1) + " omitted)") - ); - } - } - - public void testCollectionToDelimitedStringWithLimitTruncation() { - final String delimiter = randomFrom("", ",", ", ", "/"); - - final int count = between(2, 100); - final List strings = new ArrayList<>(count); - while (strings.size() < count) { - // avoid empty appends, it makes the assertions much messier - final int minLength = delimiter.isEmpty() ? 1 : 0; - strings.add(randomAlphaOfLength(between(minLength, 10))); - } - - final int fullDescriptionLength = collectionToDelimitedString(strings, delimiter).length(); - final int lastItemSize = strings.get(count - 1).length(); - final int truncatedLength = between(0, fullDescriptionLength - lastItemSize - 1); - final StringBuilder stringBuilder = new StringBuilder(); - collectionToDelimitedStringWithLimit(strings, delimiter, truncatedLength, stringBuilder); - final String truncatedDescription = stringBuilder.toString(); - - assertThat(truncatedDescription, allOf(containsString("... (" + count + " in total,"), endsWith(" omitted)"))); - - assertThat( - truncatedDescription, - truncatedDescription.length(), - lessThanOrEqualTo(truncatedLength + ("0123456789" + delimiter + "... (999 in total, 999 omitted)").length()) - ); - } - - public void testCollectionToDelimitedStringWithLimitNoTruncation() { - final String delimiter = randomFrom("", ",", ", ", "/"); - - final int count = between(1, 100); - final List strings = new ArrayList<>(count); - while (strings.size() < count) { - strings.add(randomAlphaOfLength(between(0, 10))); - } - - final String fullDescription = collectionToDelimitedString(strings, delimiter); - for (String string : strings) { - assertThat(fullDescription, containsString(string)); - } - - final int lastItemSize = strings.get(count - 1).length(); - final int minLimit = fullDescription.length() - lastItemSize; - final int limit = randomFrom(between(minLimit, fullDescription.length()), between(minLimit, Integer.MAX_VALUE), Integer.MAX_VALUE); - - final StringBuilder stringBuilder = new StringBuilder(); - collectionToDelimitedStringWithLimit(strings, delimiter, limit, stringBuilder); - assertThat(stringBuilder.toString(), equalTo(fullDescription)); - } - public void testPadStart() { String testStr; assertThat(padStart("", 5, 'a'), equalTo("aaaaa"));