Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DynamoDB ConditionalPut #5247

Merged
merged 5 commits into from Jan 4, 2024
Merged

Conversation

tustvold
Copy link
Contributor

Which issue does this PR close?

Closes #.

Rationale for this change

Builds on #4918 and extends it to conditional put.

What changes are included in this PR?

Are there any user-facing changes?

@github-actions github-actions bot added the object-store Object Store Interface label Dec 26, 2023
Comment on lines +66 to +67
/// * A string partition key named `"path"`
/// * A string sort key named `"etag"`
Copy link
Contributor Author

@tustvold tustvold Dec 26, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a breaking change, but given #4918 was only merged this morning and hasn't been released I think that is fine.

Before I had planned to encode both the path and the etag in the partition key, as was documented here, but this requires a delimiter, which after #5020 can't be #. This new approach avoids this, whilst also being I think easier to understand.

@@ -48,7 +48,7 @@ pub enum S3CopyIfNotExists {
HeaderWithStatus(String, String, reqwest::StatusCode),
/// The name of a DynamoDB table to use for coordination
///
/// Encoded as either `dynamodb:<TABLE_NAME>` or `dynamodb:<TABLE_NAME>:<TIMEOUT_MILLIS>`
/// Encoded as either `dynamo:<TABLE_NAME>` or `dynamo:<TABLE_NAME>:<TIMEOUT_MILLIS>`
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Drive by docs fix

/// $ aws dynamodb update-time-to-live --table-name <TABLE_NAME> --time-to-live-specification Enabled=true,AttributeName=ttl
/// ```
///
/// To perform a conditional operation on an object with a given `path` and `etag` (`*` if creating),
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DynamoDB doesn't let you use an empty string for a key attribute, so we use *, which is an illegal ETAG value (as If-Match wildcards would then be ambiguous)

@tustvold
Copy link
Contributor Author

@roeap when you get a chance, perhaps you could give this a look over

@tustvold
Copy link
Contributor Author

tustvold commented Jan 4, 2024

@roeap sorry to dump this on you, but I just realised this needs to go in the release or it'll be a major breaking change... Could you perhaps take a look? 🙏

Otherwise I can probably back out the other DynamoDb PR, cut the release, and then put this back

@roeap
Copy link
Contributor

roeap commented Jan 4, 2024

@tustvold - I'll take a look.

Copy link
Contributor

@roeap roeap left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 - looking good, excited to see this land.

@tustvold tustvold merged commit cf61bb8 into apache:master Jan 4, 2024
31 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
object-store Object Store Interface
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants