Test for cancelled task in TransportSnapshotsStatusAction.buildResponse()
#126740
Conversation
…se() Testing for cancellation in buildResponse() avoids a lot of unnecessary processing in scenarios with many shards. Closes ES-10981.
JeremyDahlgren
added
>non-issue
:Distributed Coordination/Snapshot/Restore
|
Pinging @elastic/es-distributed-coordination (Team:Distributed Coordination) |
...elasticsearch/action/admin/cluster/snapshots/status/TransportSnapshotsStatusActionTests.java
Show resolved
Hide resolved
...elasticsearch/action/admin/cluster/snapshots/status/TransportSnapshotsStatusActionTests.java
Outdated
Show resolved
Hide resolved
...elasticsearch/action/admin/cluster/snapshots/status/TransportSnapshotsStatusActionTests.java
Outdated
Show resolved
Hide resolved
...elasticsearch/action/admin/cluster/snapshots/status/TransportSnapshotsStatusActionTests.java
Outdated
Show resolved
Hide resolved
| assertNotNull(rsp); | ||
| final var snapshotStatuses = rsp.getSnapshots(); | ||
| assertNotNull(snapshotStatuses); | ||
| assertEquals(1, snapshotStatuses.size()); |
There was a problem hiding this comment.
For the assertEquals and assertTrue, could you add debug info strings with some more information? There are versions of the methods that take a string as the first parameter. It should help with debugability. Otherwise I think this line failing would tell us that 1 doesn't equal 2, say, but not what the extra snapshot statuses are.
| assertEquals(expectedState, snapshotStatus.getState()); | ||
| final var snapshotStatusShards = snapshotStatus.getShards(); | ||
| assertNotNull(snapshotStatusShards); | ||
| assertEquals(1, snapshotStatusShards.size()); |
| final var snapshotStatusIndices = snapshotStatus.getIndices(); | ||
| assertNotNull(snapshotStatusIndices); | ||
| assertEquals( | ||
| "expected a single entry in snapshotStatusIndices instead of " + snapshotStatusIndices, |
There was a problem hiding this comment.
Hmm, I think I see a wrinkle I didn't anticipate. IIUC, SnapshotIndexStatus doesn't have a toString method defined, so it'll print out a class name + hash code? Not ideal.
SnapshotStatus has a toString method that takes advantage of the chunking interface, variations of which SnapshotIndexStatus and SnapshotIndexShardStatus also implement. So it might be easy to write something similar for them, too.
There was a problem hiding this comment.
I added toString() methods for the relevant classes. The Strings.toString() that takes a ChunkedToXContent is deprecated. This form is the one that SnapshotStatus is currently calling. In SnapshotStatusResponse I used Strings.toString(ChunkedToXContent.wrapAsToXContent(this), true, true) directly since the assumption is we are calling toString() in scenarios where it would be reasonable to fully serialize to a String. I also set the human parameter to true in all the new toString() methods since toString() is meant to be human readable.
Testing for cancellation in
buildResponse()can avoid extra unnecessary processing in scenarios with many shards.Closes ES-10981.
This PR adds a
TransportSnapshotsStatusActionTestsunit test file to test this new functionality directly versus trying to modifying a higher level test likeSnapshotStatusApisIT. The test code added is minimal, just focusing on the changes introduced in this PR.