From 67fbbffa8ec40c3ff27af9040f97e3b7402a8083 Mon Sep 17 00:00:00 2001 From: David Turner Date: Thu, 30 Jan 2025 16:01:24 +0000 Subject: [PATCH] Cheaper snapshot-related `toString()` impls (#121283) If the `MasterService` needs to log a create-snapshot task description then it will call `CreateSnapshotTask#toString`, which today calls `RepositoryData#toString` which is not overridden so ends up calling `RepositoryData#hashCode`. This can be extraordinarily expensive in a large repository. Worse, if there's masses of create-snapshot tasks to execute then it'll do this repeatedly, because each one only ends up yielding a short hex string so we don't reach the description length limit very easily. With this commit we provide a more efficient implementation of `CreateSnapshotTask#toString` and also override `RepositoryData#toString` to protect against some other caller running into the same issue. --- docs/changelog/121283.yaml | 5 ++++ .../snapshots/SnapshotsServiceIT.java | 28 +++++++++++++++++++ .../repositories/RepositoryData.java | 5 ++++ .../snapshots/SnapshotsService.java | 5 ++++ .../repositories/RepositoryDataTests.java | 16 +++++++++++ 5 files changed, 59 insertions(+) create mode 100644 docs/changelog/121283.yaml diff --git a/docs/changelog/121283.yaml b/docs/changelog/121283.yaml new file mode 100644 index 0000000000000..56fb62acdb5fa --- /dev/null +++ b/docs/changelog/121283.yaml @@ -0,0 +1,5 @@ +pr: 121283 +summary: Cheaper snapshot-related `toString()` impls +area: Snapshot/Restore +type: bug +issues: [] diff --git a/server/src/internalClusterTest/java/org/elasticsearch/snapshots/SnapshotsServiceIT.java b/server/src/internalClusterTest/java/org/elasticsearch/snapshots/SnapshotsServiceIT.java index b9e47740e2945..b86cae1c2fb60 100644 --- a/server/src/internalClusterTest/java/org/elasticsearch/snapshots/SnapshotsServiceIT.java +++ b/server/src/internalClusterTest/java/org/elasticsearch/snapshots/SnapshotsServiceIT.java @@ -17,10 +17,12 @@ import org.elasticsearch.cluster.SnapshotDeletionsInProgress; import org.elasticsearch.cluster.metadata.IndexMetadata; import org.elasticsearch.cluster.service.ClusterService; +import org.elasticsearch.cluster.service.MasterService; import org.elasticsearch.common.settings.Settings; import org.elasticsearch.snapshots.mockstore.MockRepository; import org.elasticsearch.test.ClusterServiceUtils; import org.elasticsearch.test.MockLog; +import org.elasticsearch.test.junit.annotations.TestLogging; import java.util.List; import java.util.concurrent.TimeUnit; @@ -223,4 +225,30 @@ public void testRerouteWhenShardSnapshotsCompleted() throws Exception { safeAwait(shardMovedListener); ensureGreen(indexName); } + + @TestLogging(reason = "testing task description, logged at DEBUG", value = "org.elasticsearch.cluster.service.MasterService:DEBUG") + public void testCreateSnapshotTaskDescription() { + createIndexWithRandomDocs(randomIdentifier(), randomIntBetween(1, 5)); + final var repositoryName = randomIdentifier(); + createRepository(repositoryName, "mock"); + + final var snapshotName = randomIdentifier(); + MockLog.assertThatLogger( + () -> createFullSnapshot(repositoryName, snapshotName), + MasterService.class, + new MockLog.SeenEventExpectation( + "executing cluster state update debug message", + MasterService.class.getCanonicalName(), + Level.DEBUG, + "executing cluster state update for [create_snapshot [" + + snapshotName + + "][CreateSnapshotTask{repository=" + + repositoryName + + ", snapshot=*" + + snapshotName + + "*}]]" + ) + ); + } + } diff --git a/server/src/main/java/org/elasticsearch/repositories/RepositoryData.java b/server/src/main/java/org/elasticsearch/repositories/RepositoryData.java index 713f7eacb2c16..f7194dd6428a7 100644 --- a/server/src/main/java/org/elasticsearch/repositories/RepositoryData.java +++ b/server/src/main/java/org/elasticsearch/repositories/RepositoryData.java @@ -605,6 +605,11 @@ public int hashCode() { return Objects.hash(snapshotIds, snapshotsDetails, indices, indexSnapshots, shardGenerations, indexMetaDataGenerations); } + @Override + public String toString() { + return Strings.format("RepositoryData[uuid=%s,gen=%s]", uuid, genId); + } + /** * Resolve the index name to the index id specific to the repository, * throwing an exception if the index could not be resolved. diff --git a/server/src/main/java/org/elasticsearch/snapshots/SnapshotsService.java b/server/src/main/java/org/elasticsearch/snapshots/SnapshotsService.java index 008c75ed13473..5d8ef51af8d51 100644 --- a/server/src/main/java/org/elasticsearch/snapshots/SnapshotsService.java +++ b/server/src/main/java/org/elasticsearch/snapshots/SnapshotsService.java @@ -3885,6 +3885,11 @@ public void onFailure(Exception e) { logSnapshotFailure("create", snapshot, e); listener.onFailure(e); } + + @Override + public String toString() { + return "CreateSnapshotTask{repository=" + repository.getMetadata().name() + ", snapshot=" + snapshot + '}'; + } } private static void logSnapshotFailure(String operation, Snapshot snapshot, Exception e) { diff --git a/server/src/test/java/org/elasticsearch/repositories/RepositoryDataTests.java b/server/src/test/java/org/elasticsearch/repositories/RepositoryDataTests.java index f5ebacde08820..250d10855b23f 100644 --- a/server/src/test/java/org/elasticsearch/repositories/RepositoryDataTests.java +++ b/server/src/test/java/org/elasticsearch/repositories/RepositoryDataTests.java @@ -40,9 +40,12 @@ import static org.elasticsearch.repositories.RepositoryData.EMPTY_REPO_GEN; import static org.elasticsearch.repositories.RepositoryData.MISSING_UUID; +import static org.hamcrest.Matchers.allOf; import static org.hamcrest.Matchers.containsInAnyOrder; +import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.greaterThan; +import static org.hamcrest.Matchers.not; /** * Tests for the {@link RepositoryData} class. @@ -430,6 +433,19 @@ public void testFailsIfMinVersionNotSatisfied() throws IOException { } } + public void testToString() { + final var repositoryData = generateRandomRepoData(); + assertThat( + repositoryData.toString(), + allOf( + containsString("RepositoryData"), + containsString(repositoryData.getUuid()), + containsString(Long.toString(repositoryData.getGenId())), + not(containsString("@")) // not the default Object#toString which does a very expensive hashcode computation + ) + ); + } + public static RepositoryData generateRandomRepoData() { final int numIndices = randomIntBetween(1, 30); final List indices = new ArrayList<>(numIndices);