S3 compareAndExchange using conditional writes#139228
Conversation
DaveCTurner
left a comment
DaveCTurner
left a comment
There was a problem hiding this comment.
Implementation looks good, just a question around exposing this choice to users.
I think we need another AbstractS3RepositoryAnalysisRestTestCase subclass because there's now 3 valid configurations we should be testing:
S3ConsistencyModel.AWS_DEFAULTwith MPU danceS3ConsistencyModel.STRONG_MPUSwith MPU danceS3ConsistencyModel.AWS_DEFAULTwith new conditionalPutObject
modules/repository-s3/src/main/java/org/elasticsearch/repositories/s3/S3BlobContainer.java
Show resolved
Hide resolved
| key, | ||
| blobStore.getThreadPool() | ||
| ).run(expected, updated, ActionListener.releaseBefore(clientReference, listener)); | ||
| if (blobStore.supportsConditionalWrites(purpose)) { |
There was a problem hiding this comment.
I think we need a different config option to explicitly choose between the two implementations. Both are safe (assuming the underlying storage behaves correctly) and we want repo analysis to test the one the user actually uses, but supportsConditionalWrites(REPOSITORY_ANALYSIS) is always true so it'll never run analysis on the MPU dance.
mhl-b
added
>enhancement
:Distributed/Snapshot/Restore
|
Hi @mhl-b, I've created a changelog YAML for you. |
|
Pinging @elastic/es-distributed-coordination (Team:Distributed Coordination) |
mhl-b
requested review from
DaveCTurner,
nicktindall and
pxsalehi
and removed request for
nicktindall and
pxsalehi
| public void testMPUCompareAndExchangeCleanup() throws IOException { | ||
| final var repoMetadata = node().injector().getInstance(RepositoriesService.class).repository(TEST_REPO_NAME).getMetadata(); | ||
| final var useCasMpu = repoMetadata.settings().getAsBoolean("unsafely_incompatible_with_s3_conditional_writes", false); | ||
| assumeTrue("repository supports condtional-writes and does not use MPU for CAS", useCasMpu); |
There was a problem hiding this comment.
This makes me uneasy - are you sure this test ever runs now? I think we might just be muting this test.
The third-party tests are for running against the real S3, where this no longer makes sense, so maybe we should move this into a test suite that simply uses the fixture.
There was a problem hiding this comment.
I added random conditionals-support to third-party test. 7ebf258#diff-bc0de3c4b793cf718dd897c7f9a855a1a5f1a890b7f18755b4ed2d7a57ad3de9
testMPUCompareAndExchangeCleanup does not require strong consistency so it's ok to run against S3 and MinIO, as long as unsafely_incompatible_with_s3_conditional_writes is set. But testFailIfAlreadyExists does rely on conditionals, so I updated that test assumption too.
I think it's ok to mark specific test "not-safe" using assumptions. With this change we don't lost coverage, but test frequency.
There was a problem hiding this comment.
Another approach, I think a better one, to pass optional consistency model to "createRepository" method in every test. Without explicit argument it's random, otherwise fixed.
| // if register was deleted, return ABSENT(BytesArray.EMPTY) | ||
| return OptionalBytesReference.of(regEtag.registerContents()); | ||
| } else { | ||
| final var condtionalOperaiton = regEtag == RegisterAndEtag.ABSENT |
There was a problem hiding this comment.
typo nit
| final var condtionalOperaiton = regEtag == RegisterAndEtag.ABSENT | |
| final var condtionalOperation = regEtag == RegisterAndEtag.ABSENT |
There was a problem hiding this comment.
There is another one :) missed "i" after "cond". My French is failing me
| @@ -119,7 +121,7 @@ protected Settings repositorySettings() { | |||
| // verify we always set the x-purpose header even if disabled for other repository operations | |||
| .put(randomBooleanSetting("add_purpose_custom_query_parameter")) | |||
| // this parameter is ignored for repo analysis | |||
There was a problem hiding this comment.
nit: comment no longer applies
| // this parameter is ignored for repo analysis |
modules/repository-s3/src/main/java/org/elasticsearch/repositories/s3/S3BlobStore.java
Show resolved
Hide resolved
3 participants
Add support for S3 compareAndExchange using conditional writes. Also relax constrain on repository analysis using strict check on conditional writes. If
unsafely_incompatible_with_s3_conditional_writesis set, repository analysis will use MPU for CAS operations.