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

Add Non-conditional Put/Delete functionality #17

Merged
merged 1 commit into from
Aug 18, 2023

Conversation

G8XSU
Copy link
Collaborator

@G8XSU G8XSU commented Aug 18, 2023

Similar to popular kvstores, we should also support non-conditional put/delete operations. This opens up new patterns for usage of VSS, allows clients to easily onboard to VSS (without versioning, just for remote-backups) or to just use global_version for versioning and avoid key-level version checks.

DynamoDB: https://docs.aws.amazon.com/amazondynamodb/latest/developerguide/Expressions.ConditionExpressions.html
If-match in cosmos: https://learn.microsoft.com/en-us/rest/api/cosmos-db/replace-a-document

@G8XSU G8XSU requested a review from jkczyz August 18, 2023 06:11
@G8XSU G8XSU mentioned this pull request Aug 18, 2023
31 tasks
Copy link

@jkczyz jkczyz left a comment

Choose a reason for hiding this comment

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

How will this be exposed in the clients? Is there risk of accidentally corrupting data if a device has misconfigured its client to use non-conditional writes while another device is properly configured to do conditional writes? Is there any way to guard agains this?

app/src/main/proto/vss.proto Outdated Show resolved Hide resolved
app/src/main/proto/vss.proto Outdated Show resolved Hide resolved
app/src/main/proto/vss.proto Outdated Show resolved Hide resolved
app/src/main/proto/vss.proto Outdated Show resolved Hide resolved
app/src/main/proto/vss.proto Outdated Show resolved Hide resolved
app/src/main/proto/vss.proto Outdated Show resolved Hide resolved
Comment on lines 95 to 96
// After any non-conditional upsert, `version` against the `key` is reset to '1'. Hence the next
// `PutObjectRequest` for the `key` can either be a non-conditional upsert or a conditional write
Copy link

Choose a reason for hiding this comment

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

Or DeleteObjectRequest, too?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Since delete will delete the object, there is nothing left-over afterwards.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh, you mean delete it after put, yes we could of-course do get/list/delete as well.
Only wanted to clarify for next write operation.

app/src/main/proto/vss.proto Outdated Show resolved Hide resolved
@G8XSU
Copy link
Collaborator Author

G8XSU commented Aug 18, 2023

How will this be exposed in the clients? Is there risk of accidentally corrupting data if a device has misconfigured its client to use non-conditional writes while another device is properly configured to do conditional writes? Is there any way to guard agains this?

Yes that risk exists, this risk is similar to using single-device storage and having a multi-device app.
It can be easily circumvented by client storing config in storage. For e.g. for iphones that could be iCloud or vss.

It will be directly exposed in thin-client and will not be exposed in phase-2's PrimarySecondaryStorage.

@G8XSU G8XSU force-pushed the non-conditional-put-delete branch from b8a745c to 4984189 Compare August 18, 2023 19:04
@G8XSU G8XSU requested a review from jkczyz August 18, 2023 19:05
Copy link

@jkczyz jkczyz left a comment

Choose a reason for hiding this comment

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

Just a few minor comments. Otherwise, looks good.

app/src/main/proto/vss.proto Outdated Show resolved Hide resolved
app/src/main/proto/vss.proto Outdated Show resolved Hide resolved
app/src/main/proto/vss.proto Outdated Show resolved Hide resolved
Just like popular kvstores, we should also support non-conditional
put/delete operations. This opens up new patterns for usage of VSS, allows
clients to easily onboard to VSS (without versioning, just for remote-backups)
or to just use global_version for versioning and avoid key-level version
checks.
@G8XSU G8XSU force-pushed the non-conditional-put-delete branch from 4984189 to 14744ae Compare August 18, 2023 19:45
@G8XSU G8XSU merged commit e1a88af into lightningdevkit:main Aug 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants