Factor out common retrying logic #136663

nicktindall · 2025-10-16T06:26:46Z

Experimenting with factoring out retry logic so it can be re-used for different CSPs

Just putting this out there as a potential approach for the azure retrying. Basically it just pulls all the ES-specific retry logic out of the S3RetryingStream so we can reuse it for the CSPs.

This might be controversial. The pros/cons as I see them:

Pros

Common retry logic for Azure/S3 (and eventually GCP if we think its worthwhile)
Able to have more sophisticated retry logic (e.g. different retry behaviour per OperationPurpose, more persistent retrying when progress is being made)
Consistent delay/retry config across CSPs
Our retry logic is non-trivial, better not to duplicate it
Add a resilience feature in one place, get benefit for all 3 CSPs

Cons

The different CSP client support retries differently (retries mid-stream-read are supported in Azure out of the box)
This change introduces risk in S3 to add a feature to Azure (though I left all the S3 tests unchanged to ensure they still pass)
Separate RetryingInputStreams per CSP might be a good thing as it allows specialisation according to each store's particular quirks? (though there is still a wrapper around the client-native streams for S3/Azure where that could be put)

I tried to keep all the logic I pulled from S3RetryingInputStream -> RetryingInputStream in-tact and in-order so doing a diff it is easy to see what changed and what stayed the same.

Anyhow, I wanted to put it out there for discussion before I went much further with it to get feedback on the approach.

Update: this is probably ready for review now. The Azure and GCS implementations are both using the new common retry logic. For downloads that the retrying input stream conducts, we disable client-internal retries, so the retrying behaviour should be consistent across the board.

I apologise for the size of this PR, if it's too much I could probably split it to one per CSP

Relates: ES-13073

…reams

…tream

server/src/main/java/org/elasticsearch/common/blobstore/RetryingInputStream.java

ywangd

Overall I am in favour of this approach. The extraction looks pretty clean to me.

A few questions to help my understanding:

How feasible or how much effort to expand this to cover GoogleCloudStorageRetryingInputStream?
You said Azure supports retries mid-stream-read out of the box. What does this mean for the common retry code? Is that reopenStreamOrFail is effectively ignored by Azure?

fcofdez

Apologies for the late answer, this looks like a good direction to me 👍. As Yang mentioned, could we use this abstraction for the GCS repository too?

nicktindall · 2025-10-30T02:56:21Z

Thanks @ywangd and @fcofdez, yeah I think it will be simple to replicate this for the GCP client.

You said Azure supports retries mid-stream-read out of the box. What does this mean for the common retry code? Is that reopenStreamOrFail is effectively ignored by Azure?

I'm still not sure about that, I think the most consistent way to do things is basically set Azure to do 0 retries, and do all the retrying in our layer? I have to look at the behaviour to see whether there's anything we're missing by not using their retry capability.

ywangd · 2025-10-30T03:45:02Z

On a second thought, I don't think reopenStreamOrFail will be ignored by Azure even if it does its own functionally equivalent retries (not sure but assuming it does). We retry indefinitely for indices data so that reopenStreamOrFail should be invoked when Azure client exhausts its own retries. Basically we make the mid-retries more tenacious even if Azure client does it already.

nicktindall · 2025-10-30T04:12:16Z

On a second thought, I don't think reopenStreamOrFail will be ignored by Azure even if it does its own functionally equivalent retries (not sure but assuming it does). We retry indefinitely for indices data so that reopenStreamOrFail should be invoked when Azure client exhausts its own retries. Basically we make the mid-retries more tenacious even if Azure client does it already.

I think we should configure retries in a single layer only, as it stands (before this PR) there is no reopenStreamOrFail for Azure an we depend on the client's baked-in retries, which are less sophisticated than the S3 retries and of course don't include any special consideration of progress made or operation purpose.

ywangd · 2025-10-30T04:46:23Z

I think we should configure retries in a single layer only

While I think that's in theory preferrable, the ES level retries currently cover only read operation. So there will be gaps if we configure retries in a single layer.

nicktindall · 2025-11-20T00:44:52Z

...itory-azure/src/main/java/org/elasticsearch/repositories/azure/AzureRetryingInputStream.java

+        @Override
+        public void onRetrySucceeded(String action, long numberOfRetries) {
+            // No metrics for Azure
+        }


S3 has its own special metrics for these, we can probably make that consistent now, but I wonder if we want to do that in a separate PR to keep the volume down

nicktindall · 2025-11-20T00:47:31Z

...itory-azure/src/main/java/org/elasticsearch/repositories/azure/AzureRetryingInputStream.java

+        @Override
+        public long getMeaningfulProgressSize() {
+            return Math.max(1L, blobStore.getReadChunkSize() / 100L);
+        }


This value seems kind-of arbitrary.

The Azure value calculates to about 320KiB, the GCS to 160KiB and the S3 one to 1MiB, they are all functions of various loosely-related thresholds. Perhaps it makes sense to make this a first-class setting and consistent across the CSPs?

nicktindall · 2025-11-20T00:48:18Z

modules/repository-azure/src/main/java/org/elasticsearch/repositories/azure/AzureBlobStore.java

+            position,
+            length == null ? totalSize : length,
+            totalSize,
+            0,


We disable Azure client retries in these downloads so we can control them in the RetryingInputStream

nicktindall · 2025-11-20T00:57:28Z

...Test/java/org/elasticsearch/repositories/gcs/GoogleCloudStorageBlobStoreRepositoryTests.java

-                                .setInitialRpcTimeout(options.getRetrySettings().getInitialRpcTimeout())
-                                .setRpcTimeoutMultiplier(options.getRetrySettings().getRpcTimeoutMultiplier())
-                                .setMaxRpcTimeout(options.getRetrySettings().getMaxRpcTimeout())
                                .build()


These tests used to configure time-based retries (totalTimeout: where we retry an infinite number of times up until some time limit). The new retrying input stream doesn't support that type of configuration so I unfortunately had to change the config for this test to use attempt count limits instead. I removed a lot of the setters that just set the new value to the old value (we don't need to do that if we use oldSettings.toBuilder().

nicktindall · 2025-11-20T00:59:51Z

...sitory-gcs/src/main/java/org/elasticsearch/repositories/gcs/GoogleCloudStorageBlobStore.java

+
+    int getMaxRetries() {
+        return storageService.clientSettings(projectId, clientName).getMaxRetries();
    }


In GCS, retries are configured at the client level, not the request level. So we need to create a separate client that's configured to not retry, so we can manage retries in the RetryingInputStream. This will mean an extra client is cached for each config, but I don't think that's a huge overhead.

nicktindall · 2025-11-20T01:05:24Z

...y-gcs/src/main/java/org/elasticsearch/repositories/gcs/GoogleCloudStorageClientSettings.java

+        PREFIX,
+        "max_retries",
+        (key) -> Setting.intSetting(key, 5, 0, Setting.Property.NodeScope)
+    );


GCS didn't have any retry config previously. The default was to retry up to 5 times, with a time-limit of 50 seconds. The new default will be the same when the RetryBehaviour is ClientConfigured and we use com.google.cloud.ServiceOptions#getNoRetrySettings when RetryBehaviour equals None

nicktindall · 2025-11-20T01:10:26Z

...est/java/org/elasticsearch/repositories/gcs/GoogleCloudStorageBlobContainerRetriesTests.java

-                    retrySettingsBuilder.setMaxAttempts(maxRetries + 1);
-                }
+                    .setMaxRpcTimeout(Duration.ofSeconds(1))
+                    .setMaxAttempts(options.getRetrySettings().getMaxAttempts());


Use the setting now instead

nicktindall · 2025-11-20T01:25:38Z

...pository-s3/src/test/java/org/elasticsearch/repositories/s3/S3BlobContainerRetriesTests.java

-            purpose -> purpose == OperationPurpose.REPOSITORY_ANALYSIS || purpose == OperationPurpose.INDICES,
-            BlobStoreTestUtil::randomPurpose
-        );
-    }


These were all moved up to the parent class as they're consistent across the different CSPs

elasticsearchmachine · 2025-11-20T01:28:38Z

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

fcofdez · 2025-11-20T14:58:27Z

@nicktindall I'm not sure if I'll have the bandwidth to review this PR.

nicktindall · 2025-11-20T22:29:56Z

@nicktindall I'm not sure if I'll have the bandwidth to review this PR.

That's fine @fcofdez, I think I Yang is OK with reviewing it, I just wanted to get your opinion on the approach because you raised the original ticket. Thanks for the help.

Factor out non-CSP-specific retrying logic

33c85bc

elasticsearchmachine added the v9.3.0 label Oct 16, 2025

nicktindall added 7 commits October 17, 2025 09:39

Restore original exception throwing

419ea29

Merge remote-tracking branch 'origin/main' into retrying_blobstore_st…

7b61719

…reams

First pass AzureRetryingInputStream

643883f

Move not-found and range-not-satisfied logic into AzureRetryingInputS…

82f871a

…tream

Merge branch 'main' into retrying_blobstore_streams

c42cc7f

Create BlobRequestConditions eagerly

7e936c2

Always set ifMatch (null check is redundant)

4629790

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

nicktindall changed the title ~~Factor out non-CSP-specific retrying logic~~ Factor out common retrying logic Oct 21, 2025

nicktindall added >non-issue :Distributed Coordination/Snapshot/Restore Anything directly related to the `_snapshot/*` APIs labels Oct 21, 2025

nicktindall added 3 commits October 21, 2025 16:20

Use retrying stream to do retries

832141e

visibility

968d89b

tidy

e06d79a

nicktindall commented Oct 21, 2025

View reviewed changes

server/src/main/java/org/elasticsearch/common/blobstore/RetryingInputStream.java Show resolved Hide resolved

nicktindall requested review from fcofdez and ywangd October 21, 2025 05:31

Add test

972a505

ywangd reviewed Oct 29, 2025

View reviewed changes

fcofdez reviewed Oct 29, 2025

View reviewed changes

nicktindall added 4 commits November 7, 2025 14:15

Use common retry logic for GCS

c4fcaa1

Fix tests

3670b99

Merge branch 'main' into retrying_blobstore_streams

cec7b7f

Merge branch 'main' into retrying_blobstore_streams

2128fd1

nicktindall and others added 7 commits November 19, 2025 15:41

Fix tests again

e4b18ef

First pass on unit test

3d2c246

Tidy up tests, fix log message

c88f8e2

Minimise change

14cff38

[CI] Auto commit changes from spotless

167b29c

Test that version is requested for retries

ce4bd1f

Put some values in for "meaningful progress"

0e3ab79

nicktindall commented Nov 20, 2025

View reviewed changes

nicktindall added 2 commits November 20, 2025 11:50

Add comment indicating that getInputStream won't retry on failures

a49cbed

Minimise change

47f1834

nicktindall commented Nov 20, 2025

View reviewed changes

Use default RetrySettings from the GCS client code

5b23aa9

nicktindall commented Nov 20, 2025

View reviewed changes

nicktindall added 3 commits November 20, 2025 12:18

Make GoogleCloudStorageClientsManagerTests retry-behaviour aware

af8bc55

Tidy

86aa89f

Tidy

e3d54da

nicktindall commented Nov 20, 2025

View reviewed changes

Merge branch 'main' into retrying_blobstore_streams

792e6de

nicktindall marked this pull request as ready for review November 20, 2025 01:28

nicktindall requested a review from ywangd November 20, 2025 01:28

elasticsearchmachine added the Team:Distributed Coordination Meta label for Distributed Coordination team label Nov 20, 2025

This was referenced Nov 23, 2025

Extract generic retrying input stream logic from S3 implementation #138463

Merged

Use common retry logic for GCS #138553

Open

Factor out common retrying logic #136663

Are you sure you want to change the base?

Factor out common retrying logic #136663

Conversation

nicktindall commented Oct 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pros

Cons

Uh oh!

Uh oh!

ywangd left a comment

Choose a reason for hiding this comment

Uh oh!

fcofdez left a comment

Choose a reason for hiding this comment

Uh oh!

nicktindall commented Oct 30, 2025

Uh oh!

ywangd commented Oct 30, 2025

Uh oh!

nicktindall commented Oct 30, 2025

Uh oh!

ywangd commented Oct 30, 2025

Uh oh!

nicktindall Nov 20, 2025

Choose a reason for hiding this comment

Uh oh!

nicktindall Nov 20, 2025

Choose a reason for hiding this comment

Uh oh!

nicktindall 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!

nicktindall 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!

nicktindall Nov 20, 2025

Choose a reason for hiding this comment

Uh oh!

nicktindall 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!

nicktindall Nov 20, 2025

Choose a reason for hiding this comment

Uh oh!

nicktindall Nov 20, 2025

Choose a reason for hiding this comment

Uh oh!

elasticsearchmachine commented Nov 20, 2025

Uh oh!

fcofdez commented Nov 20, 2025

Uh oh!

nicktindall commented Nov 20, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

nicktindall commented Oct 16, 2025 •

edited

Loading

nicktindall Nov 20, 2025 •

edited

Loading

nicktindall Nov 20, 2025 •

edited

Loading

nicktindall Nov 20, 2025 •

edited

Loading