Make SnapshotResiliencyTests extensible for stateless by ywangd · Pull Request #138333 · elastic/elasticsearch

ywangd · 2025-11-20T04:56:46Z

This a pure refactor for SnapshotResiliencyTests to make it possible to be extended on the stateless side. The main changes are (1) allowing subclasses for the test class and friends; (2) extracting TestCluster related code into its own file; (3) allowing predefined executors to run as part of DeterministicQueue.

Also expand DeterministicTaskQueue

...framework/src/main/java/org/elasticsearch/common/util/concurrent/DeterministicTaskQueue.java

ywangd · 2025-11-20T05:22:24Z

...framework/src/main/java/org/elasticsearch/common/util/concurrent/DeterministicTaskQueue.java

+                final CachedSupplier<ExecutorService> supplier = predefinedExecutors.get(name);
+                if (supplier != null) {
+                    return supplier.get();


This might be controvesial. It allows a set of executors to run tasks without going through DeterministicTaskQueue. They are used to allow blocking reads to work.

Bleurgh yes controversial indeed, the whole thing stops being deterministic if we do this which defeats the point.

I haven't had a chance to dig into this and need some time to think about this more (won't be for a few hours at least) but my initial thoughts are that I'd expect the single-threaded tests to work adequately even with blocking repo operations. The thing which won't work is blocking one thread on a future while expecting other threads to do some work to complete it, and that's a pattern we should be able to (and want to) avoid here anyway.

The thing which won't work is blocking one thread on a future while expecting other threads to do some work to complete it

Yes that's exactly the problem. We have quite many such use cases in the stateless path, see example here and here. There are more.

need some time to think about this more (won't be for a few hours at least)

No rush. Even coming to just this point took me a lot of time. Thanks for looking into it.

the whole thing stops being deterministic if we do this which defeats the point

I was thinking that it would still retain the value if more interestsing work, e.g. the snapshot read/write, remains being executed deterministically. But yeah it still seems an ugly patchwork.

Yes that's exactly the problem.

Right ok, this is the exact antipattern I think we should move away from: the underlying code is async and then there's an outer layer that creates a future and then blocks on its result in order to present a sync API. Typically it's quite a mechanical change to pull these things out to the caller, and then out again, etc. until either the API becomes async again and the blocking evaporates or at least until the blocking is no longer needed for this snapshot work.

no longer needed for this snapshot change

These blocking operations are not related to snapshot. They are used more fundamentally in Stateless including recovery, refresh, flush, cluster state persistence. IIUC, they were added to satisfy Lucene's synchronous IO pattern.

I agree we want to eventually moving away from them. It seems a significant undertaking. I am OK if we decide to defer extending SnapshotResiliencyTests to stateless until we fix the blocking operations. But I think we don't want to tie it to the stateless snapshot developement. In fact, by reading shard data directly from the object store, it removes a path for blocking reading the data from the local shard.

We discussed offline and David has a great suggestion of using the direct executor for such blocking calls so that the future is ready when the blocking happens. I have updated the implementation based on this idea.

…iency-tests-to-stateless

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

…iency-tests-to-stateless

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

ywangd · 2025-12-17T08:52:51Z

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

+            return mock(PluginsService.class);
+        }
+
+        public final void init() throws IOException {


The biggest change for TestClusterNode is that initialization is split between instantation and this new init() method. Previously it was all done at object instanation time which was not feasible for extending on the serverless side.

...framework/src/main/java/org/elasticsearch/common/util/concurrent/DeterministicTaskQueue.java

…iency-tests-to-stateless

ywangd · 2025-12-18T03:17:55Z

...framework/src/main/java/org/elasticsearch/common/util/concurrent/DeterministicTaskQueue.java

-                        throw new UnsupportedOperationException();
-                    }
+        protected DeterministicThreadPool(Function<Runnable, Runnable> runnableWrapper) {
+            this.runnableWrapper = runnable -> getThreadContext().preserveContext(runnableWrapper.apply(runnable));


This is the fix for always preserving threadContext for each task. Also added a test for it in 2ad3965

Makes sense, but again I'd slightly prefer this to be a preliminary PR just to make it easier to review with confidence that it's all properly tested.

ywangd · 2025-12-18T03:21:25Z

...framework/src/main/java/org/elasticsearch/common/util/concurrent/DeterministicTaskQueue.java

-                public <T> T invokeAny(Collection<? extends Callable<T>> tasks) {
-                    throw new UnsupportedOperationException();
-                }
+    protected class DeterministicThreadPool extends ThreadPool {


The main change to DeterministicTaskQueue is pull the annoymous ThreadPool class to a named class so that it can be subclassed. I suggest viewing the diff with "Hide whitespace" to minimize the visual clutter.

DaveCTurner

Overall this looks good to me but I would like to break this change up into several steps to avoid doing too much all at once and missing something in the reviews. In particular, as well as pulling out various bits of functionality I believe this also changes the behaviour of SnapshotResiliencyTests in some (hopefully unimportant) ways. For instance:

maybeWaitForGreenAfterRestore
the injection of index settings
another stateless-only branch in testSnapshotPrimaryRelocations
delayed startup in restart()

None of this looks wrong, I'm just uncomfortable with doing a big mechanical change plus some slightly fiddly manual changes all at once. I called out in inline comments some other bits I'd prefer to be done as preliminary steps too.

DaveCTurner · 2025-12-29T16:09:25Z

test/framework/src/main/java/org/elasticsearch/cluster/ESAllocationTestCase.java

            SnapshotsInfoService snapshotsInfoService
        ) {
-            super(
+            this(


I'd rather add this constructor parameter to all the callers (there's not very many) rather than delegating to another constructor here. Could be done as a preliminary step to reduce noise.

Makes sense. I split these change to #140153

DaveCTurner · 2025-12-29T16:11:45Z

...framework/src/main/java/org/elasticsearch/common/util/concurrent/DeterministicTaskQueue.java

-                        throw new UnsupportedOperationException();
-                    }
+        protected DeterministicThreadPool(Function<Runnable, Runnable> runnableWrapper) {
+            this.runnableWrapper = runnable -> getThreadContext().preserveContext(runnableWrapper.apply(runnable));


Makes sense, but again I'd slightly prefer this to be a preliminary PR just to make it easier to review with confidence that it's all properly tested.

DaveCTurner · 2025-12-29T16:12:32Z

...framework/src/main/java/org/elasticsearch/common/util/concurrent/DeterministicTaskQueue.java

+                public ScheduledFuture<?> schedule(Runnable command, long delay, TimeUnit unit) {
+                    // For GlobalCheckpointListeners#add


This deserves more testing I think (and thus likely a separate PR)

Sure. I split changes for DeterministicQueue to its own PR #140151

The PR includes all 3 changes to the class. Please let me know if you'd like to further split it. Thanks!

DaveCTurner · 2025-12-29T16:13:39Z

...framework/src/main/java/org/elasticsearch/common/util/concurrent/DeterministicTaskQueue.java

+        protected final Function<Runnable, Runnable> runnableWrapper;

-                    @Override
-                    public ScheduledFuture<?> scheduleAtFixedRate(Runnable command, long initialDelay, long period, TimeUnit unit) {
-                        throw new UnsupportedOperationException();
-                    }
+        protected DeterministicThreadPool(Function<Runnable, Runnable> runnableWrapper) {


Nit: would prefer constructors and fields at the top of the class

They are at the top of the class. It might be a bit hidden because the instance level initialization code for forkingExecutor. I can move the initialization into the constructor if it helps. I didn't do it to minimize the change set. Please let me know.

This PR makes MockAllocationService constructor take an explicit ShardRoutingRoleStrategy parameter so that it can be customized. Relates: elastic#138333

ywangd · 2026-01-05T08:08:21Z

maybeWaitForGreenAfterRestore

the injection of index settings

another stateless-only branch in testSnapshotPrimaryRelocations

delayed startup in restart()

I raised a separate PR #140152 for just SnapshotResiliencyTests and it removes the above changes. The new PR should be more of a pure refactor. Please let me know if you spot anything for future cleanups. Thanks!

EDIT: For easy consumption, the 3 PRs splitted from this one are the follows:

This PR makes MockAllocationService constructor take an explicit ShardRoutingRoleStrategy parameter so that it can be customized. Relates: #138333

This PR makes following changes to DeterministicTaskQueue 1. Promote the anonymous ThreadPool class to a named inner class, `DeterministicThreadPool`. 2. Fix threadContext preservation for all tasks 3. Add a basic implementation for `ScheduledExecutorService#schedule(Runnable, long, TimeUnit)`. The PR is split from #138333

This PR makes following changes to DeterministicTaskQueue 1. Promote the anonymous ThreadPool class to a named inner class, `DeterministicThreadPool`. 2. Fix threadContext preservation for all tasks 3. Add a basic implementation for `ScheduledExecutorService#schedule(Runnable, long, TimeUnit)`. The PR is split from elastic#138333

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

…iency-tests-to-stateless

DaveCTurner

LGTM

DaveCTurner · 2026-01-21T06:36:12Z

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(randomFrom(dataNodes));
+            return dataNodes.isEmpty() ? Optional.empty() : Optional.ofNullable(ESTestCase.randomFrom(dataNodes));


nit: could we use qualified imports for these things?

Yeah some merge conflicts didn't resolve as they should have. This was one of them. I have now cleaned up in abb4f3b Thanks!

…iency-tests-to-stateless

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

…iency-tests-to-stateless

ywangd added 2 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

ywangd requested a review from DaveCTurner November 20, 2025 04:56

ywangd added >test Issues or PRs that are addressing/adding tests :Distributed/Snapshot/Restore Anything directly related to the `_snapshot/*` APIs v9.3.0 labels Nov 20, 2025

elasticsearchmachine added the serverless-linked Added by automation, don't add manually label Nov 20, 2025

ywangd requested a review from henningandersen November 20, 2025 05:08

license header

e310b57

ywangd commented Nov 20, 2025

View reviewed changes

...framework/src/main/java/org/elasticsearch/common/util/concurrent/DeterministicTaskQueue.java Outdated Show resolved Hide resolved

ywangd commented Nov 20, 2025

View reviewed changes

ywangd added 15 commits December 12, 2025 17:33

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

ywangd commented Dec 17, 2025

View reviewed changes

server/src/test/java/org/elasticsearch/snapshots/SnapshotResiliencyTestHelper.java Show resolved Hide resolved

ywangd commented Dec 17, 2025

View reviewed changes

...framework/src/main/java/org/elasticsearch/common/util/concurrent/DeterministicTaskQueue.java Outdated Show resolved Hide resolved

ywangd commented Dec 17, 2025

View reviewed changes

...framework/src/main/java/org/elasticsearch/common/util/concurrent/DeterministicTaskQueue.java Outdated Show resolved Hide resolved

ywangd added 3 commits December 18, 2025 12:53

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

7a48938

…iency-tests-to-stateless

add test for threadcontext preserve

2ad3965

tweak

7065937

ywangd commented Dec 18, 2025

View reviewed changes

space

64df44b

DaveCTurner reviewed Dec 29, 2025

View reviewed changes

This was referenced Jan 5, 2026

DeterministicQueue refactor and enhancement #140151

Merged

Refactor SnapshotResiliencyTests #140152

Merged

ywangd mentioned this pull request Jan 5, 2026

Refactor MockAllocationService constructor #140153

Merged

elasticsearchmachine pushed a commit that referenced this pull request Jan 6, 2026

Refactor MockAllocationService constructor (#140153)

de0fb74

This PR makes MockAllocationService constructor take an explicit ShardRoutingRoleStrategy parameter so that it can be customized. Relates: #138333

ywangd added 3 commits January 21, 2026 16:45

remove redundant changes

24c6a21

update

5fb37aa

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

cad7d71

…iency-tests-to-stateless

DaveCTurner approved these changes Jan 21, 2026

View reviewed changes

ywangd added 5 commits January 21, 2026 18:05

clean up

abb4f3b

more clean up

8e52314

one more

3984e71

restore

6090f0c

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

e2b7f44

…iency-tests-to-stateless

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

552c67c

…iency-tests-to-stateless

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

tweak

3e90b35

elasticsearchmachine merged commit ff7d81a into elastic:main Jan 22, 2026
35 checks passed

		public ScheduledFuture<?> schedule(Runnable command, long delay, TimeUnit unit) {
		// For GlobalCheckpointListeners#add

Conversation

ywangd commented Nov 20, 2025

Uh oh!

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!

DaveCTurner Nov 20, 2025 • 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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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!

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!

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 commented Jan 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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

DaveCTurner Nov 20, 2025 •

edited

Loading

ywangd commented Jan 5, 2026 •

edited

Loading