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

Support for versioned optimistic locking a la DynamoDBMapper #664

Merged
merged 1 commit into from
Aug 5, 2019

Conversation

ezmuller
Copy link
Contributor

Adds a new nullable version attribute type which initializes at 1 and increments on save/update for optimistic locking.

@ikonst
Copy link
Contributor

ikonst commented Jul 20, 2019

@garrettheel @jpinner-lyft This is a pretty well-done contribution!

We have been traditionally rejecting new features that don't map somehow to core DynamoDB features, instead pointing authors at pynamodb-attributes. However, this feature wouldn't be possible through attributes alone, since it modifies Model.save etc. Also, a version/revision attribute is a common pattern, and I can see it being useful.

What do you think?

@garrettheel
Copy link
Member

I'm supportive of putting this into the core given that this is such a common pattern. I took a quick first pass and it mostly looks good! I'll give it more of a look over the next few days.

Would love to see some docs with usage examples (especially exceptions) here too.

@ikonst
Copy link
Contributor

ikonst commented Jul 22, 2019

Ahh, I somehow missed that this maps to a DynamoDBMapper feature.

@ikonst
Copy link
Contributor

ikonst commented Jul 22, 2019

(On a side note, I'd love if we could add something like blinker signals so that such behaviors could be implemented by plugins.)

@ezmuller
Copy link
Contributor Author

The more recent update modifies the behavior of save to update the local object i.e. can be called multiple times without needing to refresh. I implemented this in Model.save since https://docs.aws.amazon.com/amazondynamodb/latest/APIReference/API_PutItem.html#DDB-PutItem-request-ReturnValues only supports ReturnValues of NONE or ALL_OLD.

I'll also update this later tonight for Transactions.

@ezmuller
Copy link
Contributor Author

Would love to see some docs with usage examples (especially exceptions) here too.

I was thinking I would wait to add docs and examples until people are happy with what I've implemented, so that I only have do it once.

For exceptions it doesn't look like I can differentiate whether a condition check failed because of the version attribute or some other conditional expression, so there won't be anything interesting there. I'll be sure to add examples with exceptions when I get there though.

@ezmuller ezmuller force-pushed the optimisticLocking branch from 2ea2617 to e43a3a6 Compare July 31, 2019 01:00
@ezmuller
Copy link
Contributor Author

Rebased and squashed to a single commit.

@ezmuller ezmuller force-pushed the optimisticLocking branch 2 times, most recently from 2f39459 to 08c7647 Compare July 31, 2019 01:52
@ikonst
Copy link
Contributor

ikonst commented Jul 31, 2019

The squash-and-force-push workflow used to make it hard to track changes, but I just realized GitHub allows now to see the diffs between the force-push and the commit it replaced.

@ezmuller
Copy link
Contributor Author

The squash-and-force-push workflow used to make it hard to track changes, but I just realized GitHub allows now to see the diffs between the force-push and the commit it replaced.

Still error-prone though. :D

@ezmuller
Copy link
Contributor Author

ezmuller commented Aug 1, 2019

@ikonst @garrettheel Any idea when you'll have time for another pass?

@garrettheel
Copy link
Member

@ezmuller docs look good, thanks for writing those! I think this is pretty much ready to go, just a couple of final small comments

@ezmuller
Copy link
Contributor Author

ezmuller commented Aug 3, 2019

@ezmuller docs look good, thanks for writing those! I think this is pretty much ready to go, just a couple of final small comments

Np feature wouldn't be all that useful undocumented. Thanks for taking the time to go through all the revisions @garrettheel @ikonst !

@ezmuller ezmuller force-pushed the optimisticLocking branch from ca5d140 to d241ef0 Compare August 4, 2019 18:19
@ezmuller
Copy link
Contributor Author

ezmuller commented Aug 4, 2019

Squashed

Copy link
Member

@garrettheel garrettheel left a comment

Choose a reason for hiding this comment

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

Thanks for all the effort here!

@garrettheel garrettheel changed the title Support for versioned optimistic locking a la DynamoDBMapper (#228) Support for versioned optimistic locking a la DynamoDBMapper Aug 5, 2019
@garrettheel garrettheel merged commit 12d8fd2 into pynamodb:master Aug 5, 2019
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.

3 participants