Support weaker consistency model for S3 MPUs#138663
Support weaker consistency model for S3 MPUs#138663DaveCTurner merged 10 commits intoelastic:mainfrom
Conversation
Adjusts the implementation of linearizable registers in S3 repositories to allow for the weaker multipart upload API semantics observed in practice. Also adjusts the S3 test fixture to (optionally) simulate the weaker semantics, and extends the repository analysis REST tests to cover both cases.
DaveCTurner
added
>bug
:Distributed/Snapshot/Restore
elasticsearchmachine
added
the
Team:Distributed Coordination (obsolete)
|
Pinging @elastic/es-distributed-coordination (Team:Distributed Coordination) |
|
Hi @DaveCTurner, I've created a changelog YAML for you. |
🔍 Preview links for changed docs |
ℹ️ Important: Docs version tagging👋 Thanks for updating the docs! Just a friendly reminder that our docs are now cumulative. This means all 9.x versions are documented on the same page and published off of the main branch, instead of creating separate pages for each minor version. We use applies_to tags to mark version-specific features and changes. Expand for a quick overviewWhen to use applies_to tags:✅ At the page level to indicate which products/deployments the content applies to (mandatory) What NOT to do:❌ Don't remove or replace information that applies to an older version 🤔 Need help?
|
Reflects the changes introduced in elastic/elasticsearch#138663.
joshua-adams-1
left a comment
joshua-adams-1
left a comment
There was a problem hiding this comment.
Looks good to me overall, just had a few small comments
| package fixture.s3; | ||
|
|
||
| /** | ||
| * AWS S3 has weaker consistency for its multipart upload APIs than initially claimed (see support cases 10837136441 and 176070774900712) |
There was a problem hiding this comment.
Side note, are these weaker consistencies to be fixed, or "features" now?
There was a problem hiding this comment.
Amazon have not indicated any interest in making changes in this area so we have to consider this a (somewhat-undocumented) feature of AWS S3 to be dealt with on our side.
| * The alternative model verified by these tests: the multipart upload APIs are strongly consistent, but the {@code If-Match} and | ||
| * {@code If-None-Match} headers are ignored and all writes are unconditional. | ||
| */ | ||
| STRONG_MPUS(false, true); |
There was a problem hiding this comment.
Is it possible for a model to have both?
There was a problem hiding this comment.
Yes, but that's not a very interesting case to test.
There was a problem hiding this comment.
Uninteresting because it should just work?
There was a problem hiding this comment.
Yes pretty much: if both of the cases here work then the (true,true) case will work automatically. Each of these flags excludes certain kinds of misbehaviour, and the point is that excluding either of those classes yields a correct result. Excluding both classes is stronger than excluding either one of them.
| } | ||
|
|
||
| protected S3ConsistencyModel consistencyModel() { | ||
| return S3ConsistencyModel.AWS_DEFAULT; |
There was a problem hiding this comment.
This value is set in both the S3HttpHandler and the S3HttpFixture. Is there a way to share it between classes?
There was a problem hiding this comment.
Not really, there are various places we use the S3HttpHandler without the surrounding fixture, but if it is running within the fixture then we have to set it like this.
Alternatively we could make it a constructor parameter and set it explicitly everywhere we create one of these things. That adds some noise to this PR so I'd rather keep it like this for now, but I do prefer constructor parameters in general so might follow up with that change later.
| } | ||
| } | ||
|
|
||
| public static void runInParallel(Runnable... tasks) { |
There was a problem hiding this comment.
[Nit] Missing Javadoc
| } | ||
|
|
||
| protected S3ConsistencyModel consistencyModel() { | ||
| return S3ConsistencyModel.AWS_DEFAULT; |
There was a problem hiding this comment.
Is there a benefit in randomising this between AWS_DEFAULT and STRONG_MPUS? (Not here, because we'd want multiple consistencyModel calls to be consistent, but as a class variable?). Would this remove the need to have the S3RepositoryAnalysisStrongMpusRestIT?
There was a problem hiding this comment.
I prefer to use randomisation for parameters that positively shouldn't matter for the purposes of the test, but this one really does matter so it's better to test both. I tried to do tricks with parameterisation etc. but it didn't work out very nicely since we construct the s3Fixture statically. There are some other options (e.g. we could make it mutable and set it during the test run) but nothing seemed totally ideal.
| * The alternative model verified by these tests: the multipart upload APIs are strongly consistent, but the {@code If-Match} and | ||
| * {@code If-None-Match} headers are ignored and all writes are unconditional. | ||
| */ | ||
| STRONG_MPUS(false, true); |
There was a problem hiding this comment.
Uninteresting because it should just work?
Reflects the changes introduced in elastic/elasticsearch#138663.
3 participants
Adjusts the implementation of linearizable registers in S3 repositories
to allow for the weaker multipart upload API semantics observed in
practice.
Also adjusts the S3 test fixture to (optionally) simulate the weaker
semantics, and extends the repository analysis REST tests to cover both
cases.