Skip to content

Commit aac1409

Browse files
authored
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.
1 parent ff3950d commit aac1409

File tree

5 files changed

+59
-0
lines changed

5 files changed

+59
-0
lines changed

docs/changelog/121283.yaml

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
pr: 121283
2+
summary: Cheaper snapshot-related `toString()` impls
3+
area: Snapshot/Restore
4+
type: bug
5+
issues: []

server/src/internalClusterTest/java/org/elasticsearch/snapshots/SnapshotsServiceIT.java

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,10 +17,12 @@
1717
import org.elasticsearch.cluster.SnapshotDeletionsInProgress;
1818
import org.elasticsearch.cluster.metadata.IndexMetadata;
1919
import org.elasticsearch.cluster.service.ClusterService;
20+
import org.elasticsearch.cluster.service.MasterService;
2021
import org.elasticsearch.common.settings.Settings;
2122
import org.elasticsearch.snapshots.mockstore.MockRepository;
2223
import org.elasticsearch.test.ClusterServiceUtils;
2324
import org.elasticsearch.test.MockLog;
25+
import org.elasticsearch.test.junit.annotations.TestLogging;
2426

2527
import java.util.List;
2628
import java.util.concurrent.TimeUnit;
@@ -223,4 +225,30 @@ public void testRerouteWhenShardSnapshotsCompleted() throws Exception {
223225
safeAwait(shardMovedListener);
224226
ensureGreen(indexName);
225227
}
228+
229+
@TestLogging(reason = "testing task description, logged at DEBUG", value = "org.elasticsearch.cluster.service.MasterService:DEBUG")
230+
public void testCreateSnapshotTaskDescription() {
231+
createIndexWithRandomDocs(randomIdentifier(), randomIntBetween(1, 5));
232+
final var repositoryName = randomIdentifier();
233+
createRepository(repositoryName, "mock");
234+
235+
final var snapshotName = randomIdentifier();
236+
MockLog.assertThatLogger(
237+
() -> createFullSnapshot(repositoryName, snapshotName),
238+
MasterService.class,
239+
new MockLog.SeenEventExpectation(
240+
"executing cluster state update debug message",
241+
MasterService.class.getCanonicalName(),
242+
Level.DEBUG,
243+
"executing cluster state update for [create_snapshot ["
244+
+ snapshotName
245+
+ "][CreateSnapshotTask{repository="
246+
+ repositoryName
247+
+ ", snapshot=*"
248+
+ snapshotName
249+
+ "*}]]"
250+
)
251+
);
252+
}
253+
226254
}

server/src/main/java/org/elasticsearch/repositories/RepositoryData.java

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -605,6 +605,11 @@ public int hashCode() {
605605
return Objects.hash(snapshotIds, snapshotsDetails, indices, indexSnapshots, shardGenerations, indexMetaDataGenerations);
606606
}
607607

608+
@Override
609+
public String toString() {
610+
return Strings.format("RepositoryData[uuid=%s,gen=%s]", uuid, genId);
611+
}
612+
608613
/**
609614
* Resolve the index name to the index id specific to the repository,
610615
* throwing an exception if the index could not be resolved.

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

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3885,6 +3885,11 @@ public void onFailure(Exception e) {
38853885
logSnapshotFailure("create", snapshot, e);
38863886
listener.onFailure(e);
38873887
}
3888+
3889+
@Override
3890+
public String toString() {
3891+
return "CreateSnapshotTask{repository=" + repository.getMetadata().name() + ", snapshot=" + snapshot + '}';
3892+
}
38883893
}
38893894

38903895
private static void logSnapshotFailure(String operation, Snapshot snapshot, Exception e) {

server/src/test/java/org/elasticsearch/repositories/RepositoryDataTests.java

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -40,9 +40,12 @@
4040

4141
import static org.elasticsearch.repositories.RepositoryData.EMPTY_REPO_GEN;
4242
import static org.elasticsearch.repositories.RepositoryData.MISSING_UUID;
43+
import static org.hamcrest.Matchers.allOf;
4344
import static org.hamcrest.Matchers.containsInAnyOrder;
45+
import static org.hamcrest.Matchers.containsString;
4446
import static org.hamcrest.Matchers.equalTo;
4547
import static org.hamcrest.Matchers.greaterThan;
48+
import static org.hamcrest.Matchers.not;
4649

4750
/**
4851
* Tests for the {@link RepositoryData} class.
@@ -430,6 +433,19 @@ public void testFailsIfMinVersionNotSatisfied() throws IOException {
430433
}
431434
}
432435

436+
public void testToString() {
437+
final var repositoryData = generateRandomRepoData();
438+
assertThat(
439+
repositoryData.toString(),
440+
allOf(
441+
containsString("RepositoryData"),
442+
containsString(repositoryData.getUuid()),
443+
containsString(Long.toString(repositoryData.getGenId())),
444+
not(containsString("@")) // not the default Object#toString which does a very expensive hashcode computation
445+
)
446+
);
447+
}
448+
433449
public static RepositoryData generateRandomRepoData() {
434450
final int numIndices = randomIntBetween(1, 30);
435451
final List<IndexId> indices = new ArrayList<>(numIndices);

0 commit comments

Comments
 (0)