Make GoogleCloudStorageRetryingInputStream request same generation on resume #127626

nicktindall · 2025-05-02T06:14:13Z

When we resume a download due to an IOException, we now specify the generation of the object that we were originally downloading in the request so that we can detect if the object changed.

Some decisions that were made

When resuming we send generation parameter rather than ifGenerationMatch. The former requests a specific generation where the latter requests the latest generation and triggers an error if its not the one specified. This distinction is only important if the bucket we're fetching from has object versioning enabled. If the object is overwritten and object versioning is enabled, resume with generation specified will succeed because the prior version will still be available, but resume with ifGenerationMatch specified would fail because the latest version is no longer the one we were downloading.
- This is similar to the behaviour of the SDK in their own resumable channels
We ignore updates to metadata. We specify the generation, which is only bumped when the object contents are updated. If someone updates the metadata that shouldn't break a resume IMO. We could implement this behaviour by using ifMetagenerationMatch or Etags instead, but I don't think we want that.

Closes ES-11432

I marked this PR as "non-issue" because currently we never update objects with different contents. This is just future proofing.

elasticsearchmachine · 2025-05-02T06:14:37Z

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

elasticsearchmachine · 2025-05-02T06:15:00Z

Hi @nicktindall, I've created a changelog YAML for you.

JeremyDahlgren

Looks good, I just had the one question about the assertion in parseGenerationHeader(), the rest were a few small nits. Might be good to get a look from other folks with GCS code experience though.

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

test/fixtures/gcs-fixture/src/main/java/fixture/gcs/MockGcsBlobStore.java

…bStore.java Co-authored-by: Jeremy Dahlgren <[email protected]>

JeremyDahlgren

LGTM

# Conflicts: # modules/repository-gcs/src/main/java/org/elasticsearch/repositories/gcs/GoogleCloudStorageRetryingInputStream.java

ywangd

LGTM

The usages for the different generation parameters seem correct to me. It would be great if we can test the change against the real thing as well. Is it not viable?

ywangd · 2025-05-20T06:12:29Z

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

+    public void testContentsChangeWhileStreaming() throws IOException {
+        GoogleCloudStorageHttpHandler handler = new GoogleCloudStorageHttpHandler("bucket");
+        httpServer.createContext("/", handler);
+        final int enoughBytesToTriggerChunkedDownload = Math.toIntExact(ByteSizeValue.ofMb(30).getBytes());


Do you mind adding a comment to explain briefly how the size of 30mb is decided?

The naming is actually incorrect (I don't think the current client does any chunking for downloads, only uploads). We just need to ensure the blob is large enough that it doesn't get entirely buffered when we read the first byte. This is probably a TCP level thing because as far as I can see the google or Java HTTP infrastructure merely relies on the underlying JVM/OS behaviour. I did some experiments locally and 2MB is usually enough to exceed the buffer, so 30M should be fairly conservative.

Done in ce03dae

ywangd · 2025-05-20T06:28:40Z

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

@@ -212,6 +217,11 @@ public HttpRequestInitializer getHttpRequestInitializer(ServiceOptions<?, ?> ser
        );
    }

+    @Override
+    protected void addSuccessfulDownloadHeaders(HttpExchange exchange) {
+        exchange.getResponseHeaders().add("x-goog-generation", "1");


If the actual generation number does not matter, can we randomize it?

Done in 2d2960c

ywangd · 2025-05-20T06:30:55Z

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

+        InetSocketAddress currentAddress = httpServer.getAddress();
+        httpServer.stop(0);
+        httpServer = MockHttpServer.createHttp(currentAddress, 0);


I wonder whether rebinding to the same address can be flaky sometimes. We can have it as is for now and see how it goes.

I think it should be OK as long as we have SO_REUSEADDR set? It seemed to work locally so perhaps that's the default?

nicktindall · 2025-05-20T12:28:16Z

It would be great if we can test the change against the real thing as well. Is it not viable?

There are assertions confirming that we do receive the x-goog-generation header and that it's the shape we expect it to be, so it should be covered by the third party tests (they download blobs using the stream).

It would be tricky to write a test where the generation failed against the real store because we'd need to somehow interrupt the stream to trigger a second request back to the server. I think that'd require some proxying or something. I think the assertions confirming the header is present and the tests simulating the resume interaction should be sufficient?

ywangd · 2025-05-20T23:37:11Z

require some proxying

Yeah that is what I had in mind, e.g. WebProxyServer. The assertion fo x-goog-generation is indeed helpful. It could be useful to test thegeneration parameter's behaviour against the real storage as well. Or maybe it is also tested elsewhere. In any case, I don't think it is necessary for this PR if at all.

For my knowledge, what is the behaviour when there is no connection interuption and the blob gets overwrite midway?

nicktindall · 2025-05-20T23:42:45Z

For my knowledge, what is the behaviour when there is no connection interuption and the blob gets overwrite midway?

That doesn't appear to be specified, I imagine as long as the download is not interrupted it would probably complete, but I haven't been able to test that. I'll see if I can run locally against the real blob store, there might be an easy way to test it.

nicktindall · 2025-05-21T04:13:36Z

Yeah that is what I had in mind, e.g. WebProxyServer.

I added an integration test using the proxy in eebcb0c

I think it's actually important to do because the documents are inconsistent about the x-goog-generation header (although it's used by the client internally), so best to have these in place. (specifically they mention that it's present in the XML API, but there's no mention of it in the JSON API that we use)

ywangd

LGTM

Thanks for adding the 3rd party test.

ywangd · 2025-05-22T00:31:08Z

...les/repository-gcs/src/test/java/org/elasticsearch/repositories/gcs/MockHttpProxyServer.java

@@ -26,19 +27,16 @@
 */
 abstract class MockHttpProxyServer implements Closeable {

-    private final HttpServer httpServer;
+    private HttpServer httpServer;


Can we make this field volatile to avoid have to think about visibility? Technically we restart the server on a generic thread and shutdown at test cleanup time in a different thread.

…tion_on_resume

…ume' into ES-11432_check_generation_on_resume

nicktindall added 2 commits May 2, 2025 14:37

Specify generation on resume

6238090

Add test for contents changed on resume

e94b246

nicktindall added >enhancement :Distributed Coordination/Snapshot/Restore Anything directly related to the `_snapshot/*` APIs labels May 2, 2025

elasticsearchmachine added Team:Distributed Coordination Meta label for Distributed Coordination team v9.1.0 labels May 2, 2025

Update docs/changelog/127626.yaml

bcab09b

Assert we get a generation header

98a80c3

nicktindall added >non-issue and removed >enhancement labels May 2, 2025

nicktindall and others added 2 commits May 2, 2025 16:33

Delete docs/changelog/127626.yaml

08e1b95

Add random path when present, don't assert generation header is present

09c6e6e

JeremyDahlgren reviewed May 2, 2025

View reviewed changes

nicktindall and others added 6 commits May 5, 2025 09:11

Update test/fixtures/gcs-fixture/src/main/java/fixture/gcs/MockGcsBlo…

576109c

…bStore.java Co-authored-by: Jeremy Dahlgren <[email protected]>

Update test/fixtures/gcs-fixture/src/main/java/fixture/gcs/MockGcsBlo…

4feb87d

…bStore.java Co-authored-by: Jeremy Dahlgren <[email protected]>

Reduce indenting

0886ba7

Assert that x-goog-generation is present

cdfffa5

Assert that x-goog-generation is present

d3c0e6a

Log a warning if we see a missing or malformed generation header

f3371e4

JeremyDahlgren approved these changes May 5, 2025

View reviewed changes

nicktindall added 2 commits May 17, 2025 16:05

Merge branch 'main' into ES-11432_check_generation_on_resume

89b926a

# Conflicts: # modules/repository-gcs/src/main/java/org/elasticsearch/repositories/gcs/GoogleCloudStorageRetryingInputStream.java

Add x-goog-generation header to test

fca2161

ywangd approved these changes May 20, 2025

View reviewed changes

nicktindall added 2 commits May 20, 2025 21:06

Randomise x-goog-generation

2d2960c

Tidy, improve naming

ce03dae

nicktindall added 2 commits May 21, 2025 14:09

Test in third-party test

eebcb0c

Merge branch 'main' into ES-11432_check_generation_on_resume

4fd9b81

nicktindall added 2 commits May 21, 2025 14:25

Clean proxied repo after test

a8826fb

Try SO_REUSEADDR to allow restarting proxy

aeea9f6

ywangd approved these changes May 22, 2025

View reviewed changes

nicktindall added 9 commits May 22, 2025 12:18

Make httpServer field volatile for thread-safe restarts

03a9b8d

Merge branch 'main' into ES-11432_check_generation_on_resume

289a594

Include repoName in base_path

a60d7e0

Add logging in proxy

b0f9298

Don't use proxy to test resume

caaf45c

Merge remote-tracking branch 'origin/main' into ES-11432_check_genera…

b800710

…tion_on_resume

Don't use proxy to test resume

0f8796b

Merge remote-tracking branch 'origin/ES-11432_check_generation_on_res…

16d6c39

…ume' into ES-11432_check_generation_on_resume

Don't use proxy to test resume

a70e2ad

nicktindall merged commit 268e39b into elastic:main May 22, 2025
19 of 20 checks passed

nicktindall deleted the ES-11432_check_generation_on_resume branch May 22, 2025 07:00

Make GoogleCloudStorageRetryingInputStream request same generation on resume #127626

Make GoogleCloudStorageRetryingInputStream request same generation on resume #127626

Uh oh!

Conversation

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

Uh oh!

Uh oh!

elasticsearchmachine commented May 2, 2025

Uh oh!

elasticsearchmachine commented May 2, 2025

Uh oh!

JeremyDahlgren left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

JeremyDahlgren left a comment

Choose a reason for hiding this comment

Uh oh!

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

nicktindall commented May 20, 2025

Uh oh!

ywangd commented May 20, 2025

Uh oh!

nicktindall commented May 20, 2025

Uh oh!

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

Uh oh!

Uh oh!

ywangd left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

nicktindall commented May 2, 2025 •

edited

Loading

nicktindall commented May 21, 2025 •

edited

Loading