Support conditional writes on versioned puts in putObjectVerCase3 - #2678
Conversation
Hello maeldonn,My role is to assist you with the merge of this Available options
Available commands
Status report is not available. |
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## development/8.5 #2678 +/- ##
===================================================
+ Coverage 74.60% 74.63% +0.02%
===================================================
Files 227 227
Lines 18578 18603 +25
Branches 3844 3849 +5
===================================================
+ Hits 13861 13884 +23
- Misses 4712 4714 +2
Partials 5 5 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
SylvainSenechal
left a comment
SylvainSenechal
left a comment
There was a problem hiding this comment.
The logic in putObjectVer3 is fine for me
Some discussion to have directly about the rejection of params.conditions
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
The following reviewers are expecting changes from the author, or must review again: |
fa9abec to
aab2ae1
Compare
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
benzekrimaha
left a comment
benzekrimaha
left a comment
There was a problem hiding this comment.
One design comment, rest looks solid (good test coverage on the condition paths).
| this.client.putObject(bucketName, objName, JSON.stringify(objVal), | ||
| log.getSerializedUids(), cb, params); | ||
| // reject rather than silently perform an unconditional write | ||
| if (params && params.conditions) { |
There was a problem hiding this comment.
no point guarding per backend: check params.conditions && !this.supportsConditionalPutObjectMD() once in MetadataWrapper.putObjectMD, that also covers the memory/file backends, which currently ignore conditions silently.
There was a problem hiding this comment.
I talked with Maël, we are just not gonna do this at all : no need to return an error, params.conditions existed before this pr and was ignored.
This pr is more of a best effort to have better database consistency, adding it to mongo for now without touching metadata is fine
ba63649 to
476f804
Compare
0a49f88 to
6675ce3
Compare
8a4259c to
4701790
Compare
4701790 to
cc047e6
Compare
| }, | ||
| ], | ||
| [ | ||
| 'should throw when two sibling fields both contain a structural operator of the same type', |
There was a problem hiding this comment.
why does this throw : it looks like a valid expression to me, should it not work?
we support (or do we actually?) a query on multiple fields: { fieldA: 4, fieldB: "foo" }, so we should support when the value on each field is an expression?
(maybe not blocking, but weird enough that we should look at it, at least in a followup...)
There was a problem hiding this comment.
I'm gonna document it in the code, its a limitation of the current algo used, but we can still express the same thing by using explicit $and
Translate params.conditions into the version document filter so callers can perform an atomic compare-and-write on an existing version. An existing version failing the condition is rejected with PreconditionFailed; duplicate key errors on the master operation remain retryable InternalError. Issue: ARSN-619
Replace the magic 11000 literal with MONGODB_DUPLICATE_KEY_ERROR across MongoClientInterface, and expand the comment on the conditional-write rejection path in putObjectVerCase3. Issue: ARSN-619
6f1754a to
46c94a3
Compare
Issue: ARSN-619
0e23858 to
d624af5
Compare
delthas
left a comment
delthas
left a comment
There was a problem hiding this comment.
LGTM. Not a fan of the pseudo abstraction we're extending here (that wants Mongo operators, so not super abstract in the end), but that's the code we have 😛
Looks like prettier is complaining. Maybe add a "run prettier" commit before all your commits first then ensure each commit is prettier-passing?
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
The following reviewers are expecting changes from the author, or must review again: |
|
/bypass_author_approval |
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
The following reviewers are expecting changes from the author, or must review again: The following options are set: bypass_author_approval |
|
I have successfully merged the changeset of this pull request
The following branches have NOT changed:
This pull request did not target the following hotfix branch(es) so they
Please check the status of the associated issue ARSN-619. Goodbye maeldonn. The following options are set: bypass_author_approval |
|
/approve |
Adds params.conditions support to putObjectVerCase3 so a version-specific put
becomes an atomic compare-and-write, failing with PreconditionFailed when the
stored version does not satisfy the condition.
Needed by CLDSRV-952.
Issue: ARSN-619