Use common retry logic for GCS by nicktindall · Pull Request #138553 · elastic/elasticsearch

nicktindall · 2025-11-25T09:06:42Z

This is the second step for breaking up and merging #136663

I chose to do GCS next because it introduces safe-resume (where we remember a version of the blob we were downloading so we can request specifically that one when we resume). This will mean less refactoring than if we'd done Azure first.

I didn't implement that logic for S3 although its trivial. I will do that in a subsequent change.

nicktindall · 2025-11-25T09:11:01Z

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

-                                .setRetryDelayMultiplier(options.getRetrySettings().getRetryDelayMultiplier())
                                .setMaxRetryDelay(Duration.ofSeconds(1L))
-                                .setMaxAttempts(0)
                                .setJittered(false)


This test originally configured retries to be time-based (i.e. no limit on the attempts, just keep retrying for some amount of time). I changed it to just make the retry intervals small and depend on the configured retry limits because we don't support time-based retries anymore.

nicktindall · 2025-11-25T09:13:59Z

...alClusterTest/java/org/elasticsearch/repositories/gcs/GoogleCloudStorageThirdPartyTests.java

            container.writeBlob(randomPurpose(), blobKey, new BytesArray(initialValue), true);

-            try (InputStream inputStream = container.readBlob(randomPurpose(), blobKey)) {
+            try (InputStream inputStream = container.readBlob(randomRetryingPurpose(), blobKey)) {


We have to be careful where we use randomPurpose() now because some purposes no longer retry (e.g. REPOSITORY_ANALYSIS)

nicktindall · 2025-11-25T09:18:51Z

.../src/main/java/org/elasticsearch/repositories/gcs/GoogleCloudStorageRetryingInputStream.java

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


The choice of this value is somewhat arbitrary, open to suggestions of whether we should make this consistent across CSPs or use some other value here. SDK default chunk size is 16MB, so this is 160KB

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

nicktindall · 2025-11-26T01:32:52Z

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

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


We never configured number of retries for GCS previously. The default settings were for 6 attempts (aka 5 retries)

…c_gcs

nicktindall · 2025-11-26T03:46:01Z

.../src/main/java/org/elasticsearch/repositories/gcs/GoogleCloudStorageRetryingInputStream.java

-    private long currentOffset;
-    private boolean closed;
-    private Long lastGeneration;
+    private static final StorageRetryStrategy STORAGE_RETRY_STRATEGY = GoogleCloudStorageService.createStorageRetryStrategy();


The one we use is stateless, we might need to re-think this lifecycle if we switch to one that is not. You can't get it out of the client or StorageOptions as far as I could see.

I don't know important this is. If necessary, I think we can store the original strategy object to the MeteredStorage object so that we can get it in this class?

I went back to using BaseService.EXCEPTION_HANDLER for the outer retries during open. This is consistent with the existing behaviour (more permissive retries on the inner loop, slightly less permissive on the outer)

…ranslate read exceptions correctly

nicktindall · 2025-11-26T05:03:22Z

.../src/main/java/org/elasticsearch/repositories/gcs/GoogleCloudStorageRetryingInputStream.java

+                }
+                return n;
+            } catch (IOException e) {
+                throw StorageException.translate(e);


We translate these for consistency with the existing implementation. We retry anything when reading, consistent with the existing implementation, but when something goes wrong the translation might add some more context in the stack-trace.

…c_gcs # Conflicts: # modules/repository-gcs/src/internalClusterTest/java/org/elasticsearch/repositories/gcs/GoogleCloudStorageBlobStoreRepositoryTests.java # modules/repository-gcs/src/test/java/org/elasticsearch/repositories/gcs/GoogleCloudStorageBlobContainerRetriesTests.java # server/src/test/java/org/elasticsearch/common/blobstore/RetryingInputStreamTests.java

…c_gcs

… use_common_retry_logic_gcs

ywangd

I have only minor comments. I don't think I'll find anything. But would like sometime to read it through again. Thanks!

ywangd · 2025-12-08T02:45:39Z

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

    @Override
    public boolean equals(Object o) {


I think we should update both equals and hashCode?

Good call, added in 735a66e

ywangd · 2025-12-08T03:03:17Z

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

            } catch (NoSuchFileException | RequestedRangeNotSatisfiedException e) {
                throw e;


We probably missed it last time? Both s3 and gcs add failures as suppressed for these two exceptions as well. If so, I think we should preserve the behaviour.

Oops, added back in 2f2ce0c

ywangd · 2025-12-08T04:14:58Z

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

-    private <T extends Exception> long maybeLogAndComputeRetryDelay(String action, T e) throws T {
-        if (shouldRetry(attempt) == false) {
+    private <T extends Exception> long maybeLogAndComputeRetryDelay(StreamAction action, T e) throws T {
+        if (blobStoreServices.isRetryableException(action, e) == false || shouldRetry(attempt) == false) {


Nit: can we move the blobStoreServices.isRetryableException check into shouldRetry as well?

Good call, moved in 0687495

ywangd · 2025-12-08T04:16:04Z

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

+    public static boolean willRetry(OperationPurpose purpose) {
+        return purpose != OperationPurpose.REPOSITORY_ANALYSIS;
+    }
+
+    public static boolean willRetryForever(OperationPurpose purpose) {
+        return purpose == OperationPurpose.INDICES;


These methods are used in tests only. So I think we should either move them to test code or somehow use them in shouldRetry to justify them being here?

Actually they're only used in one place and it's a natural home for this logic, moved them there in d444f89

ywangd · 2025-12-08T04:20:18Z

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

+                if (position < 0L) {
+                    throw new IllegalArgumentException("position must be non-negative");
+                }
+                if (length < 0) {
+                    throw new IllegalArgumentException("length must be non-negative");
+                }


I think these are already handled production code?

Yeah I just duplicated them here because I didn't want to lose that validation (we don't call super here, I just overrode so I could override getRetryDelayInMillis). I changed them to assertions in 2f7cd06

If I've understood correctly?

I meant that the same checks are done in RetryingInputStream's constructor which is invoked in the else branch. That said, it's not called when length == 0. Assertions are fine.

ywangd · 2025-12-08T04:23:17Z

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

    protected OperationPurpose randomRetryingPurpose() {
-        return randomValueOtherThan(OperationPurpose.REPOSITORY_ANALYSIS, BlobStoreTestUtil::randomPurpose);
+        return BlobStoreTestUtil.randomRetryingPurpose();
    }

    @Override
    protected OperationPurpose randomFiniteRetryingPurpose() {
-        return randomValueOtherThanMany(
-            purpose -> purpose == OperationPurpose.REPOSITORY_ANALYSIS || purpose == OperationPurpose.INDICES,
-            BlobStoreTestUtil::randomPurpose
-        );
+        return BlobStoreTestUtil.randomFiniteRetryingPurpose();
    }


Can't we have these methods defined in AbstractBlobContainerRetriesTestCase? Same for GoogleCloudStorageBlobContainerRetriesTests?

They're now also used in classes that aren't descendants of AbstractBlobContainerRetriesTestCase, so that reuse is easier if they're available as static utilities.

The old pattern was to have them defined in AbstractBlobContainerRetriesTestCase and overridden in the subclasses because the operation-purpose specific logic was unique to S3. Once we get this and the Azure implementation implemented the behaviour will be consistent and we can not bother with the inheritance and overrides and just use the static methods everywhere.

ywangd · 2025-12-08T04:29:19Z

server/src/test/java/org/elasticsearch/common/blobstore/RetryingInputStreamTests.java

+                if (getAttempts() > 1) {
+                    assertEquals(eTag, version);


Is this branch not tested since maxRetries is 0?

It was, because I had used OperationPurpose.INDICES which retries infinitely regardless of the configured max retries. I changed it to use a random retrying purpose and configured max retries accordingly. It confused even me when I looked at it again :)

see 52f8659

…ptions

ywangd

I have two more questions.

ywangd · 2025-12-12T01:09:30Z

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

            if (offset > 0 || start > 0 || end < Long.MAX_VALUE - 1) {
                assert start + offset <= end : "requesting beyond end, start = " + start + " offset=" + offset + " end=" + end;
            }
+            // noinspection TryWithIdenticalCatches


ywangd · 2025-12-12T01:34:18Z

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

            } catch (RuntimeException e) {
-                if (attempt == 1) {
-                    blobStoreServices.onRetryStarted("open");
-                }
-                final long delayInMillis = maybeLogAndComputeRetryDelay("opening", e);
-                delayBeforeRetry(delayInMillis);
+                retryOrAbortOnOpen(e);
+            } catch (IOException e) {
+                retryOrAbortOnOpen(e);
            }


It seems impossible for GoogleCloudStorageRetryingInputStream to throw IOException on openning other than NoSuchFileException and RequestedRangeNotSatisfiedException which are handled separately. S3RetryingInputStream currently does not retry on IOException. If so, do we need this separate handling of IOException? Or is it for future Azure changes?

Yeah the Azure implementation does throw IOException if we migrate it as-is, but looking at it it's really just used as a catch-all. I'd be keen to minimise the differences until the dust has settled on these changes.

Also as long as BlobStoreServices#getInputStream throws IOException static analysis will require us to keep it.

Perhaps after Azure, we can change those exceptions to be more specific?

.../src/main/java/org/elasticsearch/repositories/gcs/GoogleCloudStorageRetryingInputStream.java

…ories/gcs/GoogleCloudStorageRetryingInputStream.java Co-authored-by: Yang Wang <ywangd@gmail.com>

ywangd

LGTM

Use common retry logic for GCS

1ebbae2

elasticsearchmachine added the v9.3.0 label Nov 25, 2025

nicktindall commented Nov 25, 2025

View reviewed changes

nicktindall added :Distributed/Snapshot/Restore Anything directly related to the `_snapshot/*` APIs >enhancement >non-issue labels Nov 25, 2025

nicktindall added 4 commits November 25, 2025 20:25

Merge branch 'main' into use_common_retry_logic_gcs

915be8a

Use FilterInputStream for SingleAttemptInputStream (delegate close etc.)

709fdf1

Account for bytes skipped in offset

f0fca99

Make SingleAttemptInputStream a decorator

a078ff8

nicktindall commented Nov 26, 2025

View reviewed changes

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

nicktindall commented Nov 26, 2025

View reviewed changes

nicktindall added 3 commits November 26, 2025 14:24

Allow CSPs to define what is retry-able

39f8db6

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

f5502d5

…c_gcs

Remove forbidden API call (toLowerCase)

1f4b905

nicktindall commented Nov 26, 2025

View reviewed changes

nicktindall added 4 commits November 26, 2025 14:47

Remove misleading comments

b70d52f

Update main javadoc to indicate why this class exists

5c03349

Improve Javadoc for RetryingInputStream

4981c7e

Remove copied config, configure randomXXXPurpose correctly for GCP, t…

fabe579

…ranslate read exceptions correctly

nicktindall commented Nov 26, 2025

View reviewed changes

nicktindall added 7 commits November 26, 2025 16:53

Fix metric value

d137a49

Override retry delay for retry tests

55c98eb

Align behaviour with current

732121f

Merge branch 'main' into use_common_retry_logic_gcs

9da242f

Use retrying purpose when we expect reads to succeed

63d6018

Merge branch 'main' into use_common_retry_logic_gcs

f6c2497

nicktindall added 8 commits December 1, 2025 10:03

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

e50aa3f

…c_gcs

Merge remote-tracking branch 'origin/use_common_retry_logic_gcs' into…

33a2751

… use_common_retry_logic_gcs

Leave implementation with retrying in-client and in the wrapper

dc9454e

Update comment clarifying that we have two layers of retries

5c3965b

Remove redundant comment

f3ffdd3

Leave qualifier in front of EXCEPTION_HANDLER

14a109f

Merge branch 'main' into use_common_retry_logic_gcs

eb6d8d5

Merge branch 'main' into use_common_retry_logic_gcs

737cf90

nicktindall requested a review from ywangd December 1, 2025 22:51

ywangd reviewed Dec 8, 2025

View reviewed changes

nicktindall added 8 commits December 10, 2025 17:28

Update equals and hashCode to include maxRetries

735a66e

Add suppressed exceptions on no such file or range not satisfied exce…

2f2ce0c

…ptions

Consolidate shouldRetry logic

0687495

In-line willRetry and willRetryForever

d444f89

Change validation to assertions

2f7cd06

Use potentially finite retries for blob version test

52f8659

Use short, fixed delay

16bbca6

Merge branch 'main' into use_common_retry_logic_gcs

5ba165b

nicktindall requested a review from ywangd December 10, 2025 07:31

Merge branch 'main' into use_common_retry_logic_gcs

51abf0c

ywangd reviewed Dec 12, 2025

View reviewed changes

nicktindall and others added 5 commits December 12, 2025 14:07

Update modules/repository-gcs/src/main/java/org/elasticsearch/reposit…

ca4786c

…ories/gcs/GoogleCloudStorageRetryingInputStream.java Co-authored-by: Yang Wang <ywangd@gmail.com>

Implement retries for skip

80624e5

Merge branch 'main' into use_common_retry_logic_gcs

a20e5d5

Use read for skip in RetryingInputStream

c843308

Merge branch 'main' into use_common_retry_logic_gcs

587757a

ywangd approved these changes Dec 12, 2025

View reviewed changes

nicktindall merged commit c0e1c56 into elastic:main Dec 12, 2025
34 checks passed

nicktindall deleted the use_common_retry_logic_gcs branch December 12, 2025 07:05

nicktindall mentioned this pull request Jan 9, 2026

Factor out common retrying logic #136663

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use common retry logic for GCS#138553

Use common retry logic for GCS#138553
nicktindall merged 44 commits intoelastic:mainfrom
nicktindall:use_common_retry_logic_gcs

nicktindall commented Nov 25, 2025 •

edited

Loading

nicktindall Nov 25, 2025 •

edited

Loading

nicktindall Nov 25, 2025 •

edited

Loading

nicktindall Nov 25, 2025

Uh oh!

nicktindall Nov 26, 2025

nicktindall Nov 26, 2025

ywangd Nov 28, 2025

nicktindall Dec 1, 2025

nicktindall Nov 26, 2025 •

edited

Loading

ywangd left a comment

ywangd Dec 8, 2025

nicktindall Dec 10, 2025

ywangd Dec 8, 2025

nicktindall Dec 10, 2025

ywangd Dec 8, 2025

nicktindall Dec 10, 2025

ywangd Dec 8, 2025

nicktindall Dec 10, 2025

ywangd Dec 8, 2025

nicktindall Dec 10, 2025

ywangd Dec 12, 2025

ywangd Dec 8, 2025

nicktindall Dec 10, 2025

ywangd Dec 8, 2025

nicktindall Dec 10, 2025

ywangd left a comment

ywangd Dec 12, 2025

ywangd Dec 12, 2025

nicktindall Dec 12, 2025

Uh oh!

ywangd left a comment

Uh oh!

Labels

4 participants

		} catch (NoSuchFileException \| RequestedRangeNotSatisfiedException e) {
		throw e;

Conversation

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

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

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

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

Uh oh!

Choose a reason for hiding this comment

ywangd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ywangd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Uh oh!

ywangd left a comment

Choose a reason for hiding this comment

Uh oh!

Labels

4 participants

nicktindall commented Nov 25, 2025 •

edited

Loading

nicktindall Nov 25, 2025 •

edited

Loading

nicktindall Nov 25, 2025 •

edited

Loading

nicktindall Nov 26, 2025 •

edited

Loading