Refactor SnapshotResiliencyTests by ywangd · Pull Request #140152 · elastic/elasticsearch

ywangd · 2026-01-05T07:54:21Z

Refactor for SnapshotResiliencyTests to make it easier to extend. The main changes are (1) extract TestCluster related code into its own file and (2) add a separate init phase to the test node so that it can be customized by future subclasses. It also extracts serveral protected methods from existing code so they can be customized in future as well.

Relates: #138333

Also expand DeterministicTaskQueue

…iency-tests-to-stateless

Also support waiting for a condition before starting the restart node.

…iency-tests-to-stateless

…-resiliency-tests-to-stateless

elasticsearchmachine · 2026-01-05T07:54:46Z

Pinging @elastic/es-distributed-coordination (Team:Distributed Coordination)

DaveCTurner

Looks broadly good to me, just a few questions/nits.

DaveCTurner · 2026-01-05T09:50:30Z

server/src/test/java/org/elasticsearch/snapshots/SnapshotResiliencyTestHelper.java

+                }
+                return true;
+            }).sorted(Comparator.comparing(n -> n.node.getName())).toList();
+            return dataNodes.isEmpty() ? Optional.empty() : Optional.ofNullable(ESTestCase.randomFrom(dataNodes));


nit: slight preference for qualified imports rather than qualifying the method calls

Suggested change

return dataNodes.isEmpty() ? Optional.empty() : Optional.ofNullable(ESTestCase.randomFrom(dataNodes));

return dataNodes.isEmpty() ? Optional.empty() : Optional.ofNullable(randomFrom(dataNodes));

Sure pushed 94a9744

DaveCTurner · 2026-01-05T09:52:56Z

server/src/test/java/org/elasticsearch/snapshots/SnapshotResiliencyTestHelper.java

+        }
+    }
+
+    public static class TestClusterNode {


Why raise this to a first-level inner static class rather than remaining an inner class of TestClusterNodes? I'm guessing it's so that we can subclass it later? But if you're in a subclass of TestClusterNodes can you not subclass TestClusterNode? Haven't tried it, just curious really.

I sorta generally prefer top level classes. But it is also ok to keep it as an inner class of TestClusterNodes. We can still subclass it as you suggested which is what happened for DeterministicQueue#DeterministicThreadPool. So I changed it accordingly in 9d6806d
Since it touches the constructor, the change also moved both nodeSettings() and createPluginsService() methods to TestClusterNodes so that we can remove the this-escape.

Let me know if you want TestClusterNodes to be put back into SnapshotResiliencyTests. I'd like to still make it a static class in that case. That said, I find it overall more manageable with two separate files since both of them have quite large sizes and we could potentially use the test cluster for other things in future.

DaveCTurner · 2026-01-05T10:03:10Z

server/src/test/java/org/elasticsearch/snapshots/SnapshotResiliencyTestHelper.java

+            actions.put(
+                TransportClusterHealthAction.TYPE,


Do we need to register this action now? Seems like a little more than a pure refactor if so. If not, can we delay it until it's needed?

I removed this action registration along with the doInit method in 1dbc330

DaveCTurner · 2026-01-05T10:04:24Z

server/src/test/java/org/elasticsearch/snapshots/SnapshotResiliencyTestHelper.java

+                )
+            );
+
+            doInit(actions, actionFilters);


Likewise here, this hook doesn't do anything yet and adding it will be relatively noise-free later.

DaveCTurner · 2026-01-05T10:06:06Z

server/src/test/java/org/elasticsearch/snapshots/SnapshotResiliencyTestHelper.java

+
+        private Coordinator coordinator;
+
+        @SuppressWarnings("this-escape")


Does this really allow a this to escape, given that we now have a separate init() method?

Sorry I see now, I thought it picked this up earlier in the precommit run than it did. The issue is the calls to the instance methods nodeSettings() and createPluginsService(), which do not leak this here but might do so in a subclass I guess. But I wonder, do we need these to be overridable by subclasses or could we pass them in as contructor parameters instead?

As commented above, it is removed in 9d6806d

DaveCTurner · 2026-01-05T10:08:31Z

server/src/test/java/org/elasticsearch/snapshots/SnapshotResiliencyTests.java


        runUntil(documentCountVerified::get, TimeUnit.MINUTES.toMillis(5L));
        assertNotNull(safeResult(createSnapshotResponseListener));
-        assertNotNull(safeResult(restoreSnapshotResponseListener));


Hmm do we need to drop this assertion?

It was a mistake. Restored in 8970408 Thanks for catching it.

DaveCTurner · 2026-01-05T10:10:29Z

server/src/test/java/org/elasticsearch/snapshots/SnapshotResiliencyTests.java

+                .filter(ShardRouting::primary)
+                .findFirst()
+                .orElseThrow(() -> new AssertionError("no primary shard found"));


Slight preference for deferring this change in behaviour.

Sure, restored in 266bd8b

DaveCTurner · 2026-01-05T10:13:27Z

server/src/test/java/org/elasticsearch/snapshots/SnapshotResiliencyTestHelper.java

+                null,
+                emptySet()
+            );
+            // TODO: The indexNameExpressionResolver does not use the same threadContext and projectResolver


Is this important? It sounds important...

Practically it is not important since the tests do not rely on either multi-project or system indices. I didn't touch it because there are other inconsistencies with projectResolver and I was not sure whether it was the right PR to fix them. Since we now have these changes separately. I pushed 4b5d66a to fix them.

Also removed maybeExecuteTransportRunnable

Removed this-escape and some adjustments for constructor

…ests-refactor

ywangd · 2026-01-08T07:03:12Z

@DaveCTurner This PR is ready for another look when you have time. Thanks!

ywangd · 2026-01-15T07:14:50Z

@DaveCTurner Sorry to ping you again. Your review is appreciated when convenient. Thanks a lot!

DaveCTurner

Looks good now, thanks Yang

DaveCTurner · 2026-01-19T06:22:41Z

server/src/test/java/org/elasticsearch/snapshots/SnapshotResiliencyTests.java


        continueOrDie(clusterStateResponseStepListener, clusterStateResponse -> {
            final ShardRouting shardToRelocate = clusterStateResponse.getState().routingTable().allShards(index).get(0);
+            ;


nit: looks like a typo here

Good catch! Removed in f15e670

…ests-refactor

Refactor for `SnapshotResiliencyTests` to make it easier to extend. The main changes are (1) extract TestCluster related code into its own file and (2) add a separate init phase to the test node so that it can be customized by future subclasses. It also extracts serveral protected methods from existing code so they can be customized in future as well. Relates: elastic#138333

ywangd added 28 commits November 20, 2025 00:22

Refactor SnapshotResiliencyTests to make it extendable to stateless

be339bf

Also expand DeterministicTaskQueue

Add support for search node and search shard

1a78a34

license header

e310b57

Merge remote-tracking branch 'origin/main' into extend-snapshot-resil…

4c7b99e

…iency-tests-to-stateless

Merge remote-tracking branch 'origin/main' into extend-snapshot-resil…

a6a8708

…iency-tests-to-stateless

revert changes for DeterministicTaskQueue

4a016bf

Use direct executor

e979538

fix reconfigurator

9c42bda

fix number of default shards

cb28156

fix wait for green after restore

2786565

adapt for async translog upload

3bd2adc

fix testSuccessfulSnapshotWithConcurrentDynamicMappingUpdates

c129b32

fix testSnapshotDeleteWithMasterFailover

c3e7b07

Expand warning headers for merge scheduler

d710ad0

add stateless variant branch for testSnapshotPrimaryRelocations

9226f1c

Fix restart by creating node of the right subclass.

2bc9639

Also support waiting for a condition before starting the restart node.

Merge remote-tracking branch 'origin/main' into extend-snapshot-resil…

9143f32

…iency-tests-to-stateless

tweak for less change

14290c9

Remove extension points and allow the ThreadPool itself to be subclassed

f65346b

Ensure threadContext is preserved for all tasks

feab33a

Merge remote-tracking branch 'origin/main' into extend-snapshot-resil…

7a48938

…iency-tests-to-stateless

add test for threadcontext preserve

2ad3965

tweak

7065937

space

64df44b

Merge remote-tracking branch 'origin/main' into split-extend-snapshot…

2ecdea3

…-resiliency-tests-to-stateless

remove unnecessary changes

c856c75

remove future changes from snapshot resiliency tests

996e012

Remove unwanted changes

3faa672

ywangd requested a review from DaveCTurner January 5, 2026 07:54

ywangd added the >test Issues or PRs that are addressing/adding tests label Jan 5, 2026

ywangd added :Distributed/Snapshot/Restore Anything directly related to the `_snapshot/*` APIs v9.4.0 labels Jan 5, 2026

elasticsearchmachine added the Team:Distributed Coordination (obsolete) Meta label for Distributed Coordination team. Obsolete. Please do not use. label Jan 5, 2026

ywangd mentioned this pull request Jan 5, 2026

Make SnapshotResiliencyTests extensible for stateless #138333

Merged

DaveCTurner reviewed Jan 5, 2026

View reviewed changes

ywangd added 8 commits January 6, 2026 11:26

qualified import

94a9744

remove registration of TransportClusterHealthAction and doInit method

1dbc330

Also removed maybeExecuteTransportRunnable

Move TestClusterNode as an inner class of TestClusterNodes

9d6806d

Removed this-escape and some adjustments for constructor

restore assertion

8970408

restore primary shard filtering

266bd8b

tidy up projectResolver usage

4b5d66a

Merge remote-tracking branch 'origin/main' into snapshot-resiliency-t…

0f0708d

…ests-refactor

tweak

be820b0

ywangd requested a review from DaveCTurner January 6, 2026 02:16

Merge branch 'main' into snapshot-resiliency-tests-refactor

34e96e7

Merge branch 'main' into snapshot-resiliency-tests-refactor

6c1d30e

DaveCTurner approved these changes Jan 19, 2026

View reviewed changes

ywangd added 2 commits January 20, 2026 10:13

Merge remote-tracking branch 'origin/main' into snapshot-resiliency-t…

e9c370a

…ests-refactor

redundant semicolon

f15e670

ywangd added the auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) label Jan 19, 2026

elasticsearchmachine merged commit ba61a61 into elastic:main Jan 20, 2026
35 checks passed

ywangd deleted the snapshot-resiliency-tests-refactor branch January 20, 2026 01:11

DaveCTurner mentioned this pull request Jan 20, 2026

Fix SnapshotResiliencyTests#testSnapshotWithNodeDisconnects #140960

Merged

	return dataNodes.isEmpty() ? Optional.empty() : Optional.ofNullable(ESTestCase.randomFrom(dataNodes));
	return dataNodes.isEmpty() ? Optional.empty() : Optional.ofNullable(randomFrom(dataNodes));


		private Coordinator coordinator;

		@SuppressWarnings("this-escape")

Conversation

ywangd commented Jan 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticsearchmachine commented Jan 5, 2026

Uh oh!

DaveCTurner left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ywangd Jan 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ywangd Jan 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ywangd Jan 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ywangd commented Jan 8, 2026

Uh oh!

ywangd commented Jan 15, 2026

Uh oh!

DaveCTurner left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ywangd commented Jan 5, 2026 •

edited

Loading

ywangd Jan 6, 2026 •

edited

Loading

ywangd Jan 6, 2026 •

edited

Loading

ywangd Jan 6, 2026 •

edited

Loading