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 TransactWriteItems #48

Merged
merged 4 commits into from
May 13, 2022
Merged

Conversation

bartelink
Copy link
Member

@bartelink bartelink commented May 9, 2022

  • Wraps the TransactWriteItems API

    • Basic impl
    • Validate IRL in Equinox Add DynamoDB jet/equinox#321
    • empty request list or >25 items should be rejected with ArgumentOutOfRangeException
      • arguably all such guards should be made consistent herein?
    • Tests
      • passing/failing Check + unconditional PutItem pair
      • Cover presence and absence of all conditions and all actions
      • gathering metrics (not available when check fails)
    • README example
  • Adds a Precondition.CheckFailed for trapping normal precondition check fails

@bartelink
Copy link
Member Author

@samritchie This develops bartelink#3
I'm flexible on whether the cleanup PR #47 goes in before or after this (or in some other release)

Will keep chipping away adding tests but would appreciate your thoughts on where to draw the line - ultimately I'm satisfied the basic functionality of the exception filter and the condition checks etc are correct.

After that, the incremental value of validating arbitrary combinations of conditions and individual TransactWrite items becomes more marginal for me. i.e. its duplicating business of the underlying service and/or SDK (which is not the end of the world, but in the context of this repo they should be marked as characterisation tests and/or examples)

@bartelink
Copy link
Member Author

@samritchie I can't think of anything else to do so from my point of view this is ready for final review - let me know if you can see anything that needs doing (in particular, the README is more a braindump than polished tech writing!)

if not, I'll integrate test it when you get a chance to merge and release

@bartelink bartelink force-pushed the transact-write branch 5 times, most recently from 1448129 to 45fe9ba Compare May 10, 2022 04:38
@samritchie
Copy link
Collaborator

Thanks @bartelink, I'll go through this today - I’m still recovering from the plague and my brain is a bit slow at the moment

@bartelink
Copy link
Member Author

Thanks - I know the feeling so doubly appreciate any time you can carve out in that case.

For reference, here's where it all slots into Equinox (am I glad I have tests over that!) https://github.com/jet/equinox/blob/6a23d9e26aab33b027d7694502415a552e9ce5bb/src/Equinox.DynamoStore/DynamoStore.fs#L417-L468

(And if you think no reasonable person could love that code, contrast it with https://github.com/jet/equinox/blob/6a23d9e26aab33b027d7694502415a552e9ce5bb/src/Equinox.CosmosStore/CosmosStore.fs#L362-L469, which definitely took more development effort and tight-rope walking!)

@bartelink bartelink mentioned this pull request May 11, 2022
36 tasks
@bartelink
Copy link
Member Author

Nudge @samritchie - hoping to release a beta of jet/equinox#321 vs having to hand-build some alpha... Even an -alpha Nuget upload would help a lot...

Copy link
Collaborator

@samritchie samritchie left a comment

Choose a reason for hiding this comment

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

I'm happy to merge & release this - there’s certainly value here for the single-table crowd.

However this library doesn’t cater particularly well for that pattern & I don’t know how many of those users there are. I expect more than one person will end up wasting time trying to work out out how to do TransactWrite across multiple tables - it feels a little incomplete.

The docs are a bit wordy and are heavy on third party links - have put some comments in there but let’s tidy that up when we have time.

src/FSharp.AWS.DynamoDB/TableContext.fs Show resolved Hide resolved

do! table.TransactWriteItems requests

let! itemFound = table.ContainsKeyAsync (table.Template.ExtractKey item)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not overly enthusiastic about the use of things like template.ExtractKey in the tests, purely because new users often look at tests for usage guidance, and I view the record template as internal-ish. Don’t do anything with this for now though, I will have a more general review of the tests.

Copy link
Member Author

Choose a reason for hiding this comment

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

Will be interested to see what your alternative is; are you suggesting having a property on the record that calls TableKey.Combined explicitly?

README.md Show resolved Hide resolved
README.md Show resolved Hide resolved
@samritchie samritchie merged commit 1747b3c into fsprojects:master May 13, 2022
@bartelink
Copy link
Member Author

Thanks for releasing, much appreciated. Your comments are on the money; will circle back on them in the coming days...

@bartelink bartelink deleted the transact-write branch July 13, 2022 13:10
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