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 DynamoDBHistoryAutoIncrement class for version control in DynamoDB #20

Merged
merged 18 commits into from
Nov 5, 2023

Conversation

dakota002
Copy link
Contributor

This replaces #16 and #17

@codecov
Copy link

codecov bot commented Oct 11, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (230a672) 92.59% compared to head (2acc2e2) 100.00%.
Report is 1 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff             @@
##             main       #20      +/-   ##
===========================================
+ Coverage   92.59%   100.00%   +7.40%     
===========================================
  Files           1         1              
  Lines          27        39      +12     
  Branches        3         5       +2     
===========================================
+ Hits           25        39      +14     
+ Misses          1         0       -1     
+ Partials        1         0       -1     
Files Coverage Δ
src/index.ts 100.00% <100.00%> (+7.40%) ⬆️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

src/index.ts Outdated Show resolved Hide resolved
src/index.ts Outdated Show resolved Hide resolved
lpsinger added a commit to lpsinger/dynamodb-autoincrement that referenced this pull request Oct 16, 2023
lpsinger added a commit to lpsinger/dynamodb-autoincrement that referenced this pull request Oct 17, 2023
@lpsinger
Copy link
Member

@dakota002, I've come around to your original idea. It has many advantages over mine:

  • It uses half as much storage space in DynamoDB.
  • It will require no database migration.
  • In both the Circular auto increment application and the version numbering application, only two tables are involved, not three.

However it does add a bunch of conditional logic, which is a hint that we should be pulling out the common logic and reusing it in two new classes. I am going to propose a couple refactorings in separate PRs that will make this more elegant.

lpsinger added a commit to lpsinger/dynamodb-autoincrement that referenced this pull request Oct 31, 2023
This makes it easier to create different autoincrement variations
with the same optimistic locking loop (e.g. nasa-gcn#20).
lpsinger added a commit to lpsinger/dynamodb-autoincrement that referenced this pull request Oct 31, 2023
This makes it easier to create different autoincrement variations
with the same optimistic locking loop (e.g. nasa-gcn#20).
lpsinger added a commit that referenced this pull request Oct 31, 2023
This makes it easier to create different autoincrement variations
with the same optimistic locking loop (e.g. #20).
lpsinger added a commit to lpsinger/dynamodb-autoincrement that referenced this pull request Oct 31, 2023
In practice, the version or counter attribute has the same name in
both tables. Forcing that to be the case will make it simpler to
implement versioning (nasa-gcn#20).
@lpsinger
Copy link
Member

lpsinger commented Nov 1, 2023

At this point, @dakota002, please implement your original versioning algorithm, but by subclassing the base class.

Copy link
Member

@lpsinger lpsinger left a comment

Choose a reason for hiding this comment

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

What I'm looking for is a new class that extends BaseDynamoDBAutoIncrement and implements the next method. You shouldn't need to modify the existing DynamoDBAutoIncrement class, and you shouldn't need to add a flag to the props.

@dakota002
Copy link
Contributor Author

What I'm looking for is a new class that extends BaseDynamoDBAutoIncrement and implements the next method. You shouldn't need to modify the existing DynamoDBAutoIncrement class, and you shouldn't need to add a flag to the props.

Sounds good!

src/index.ts Outdated Show resolved Hide resolved
src/index.ts Outdated Show resolved Hide resolved
src/index.ts Outdated Show resolved Hide resolved
src/index.ts Outdated Show resolved Hide resolved
src/index.ts Outdated Show resolved Hide resolved
src/index.ts Outdated Show resolved Hide resolved
src/index.ts Outdated Show resolved Hide resolved
dakota002 and others added 4 commits November 2, 2023 09:39
Co-authored-by: Leo Singer <[email protected]>
Co-authored-by: Leo Singer <[email protected]>
Co-authored-by: Leo Singer <[email protected]>
Co-authored-by: Leo Singer <[email protected]>
src/index.ts Outdated Show resolved Hide resolved
src/index.ts Outdated Show resolved Hide resolved
src/index.ts Outdated Show resolved Hide resolved
Copy link
Member

@lpsinger lpsinger left a comment

Choose a reason for hiding this comment

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

  • Please add a unit test that is analogous to correctly handles a large number of parallel puts, that tests that the locking is working correctly.

src/index.ts Outdated Show resolved Hide resolved
src/index.ts Outdated Show resolved Hide resolved
src/index.ts Outdated Show resolved Hide resolved
src/test.ts Outdated Show resolved Hide resolved
@lpsinger lpsinger merged commit bec54aa into nasa-gcn:main Nov 5, 2023
8 of 9 checks passed
@lpsinger lpsinger changed the title Does the versioning stuff, using the Puts Add DynamoDBHistoryAutoIncrement class for version control in DynamoDB Nov 5, 2023
@dakota002 dakota002 deleted the 3rdTry branch November 5, 2023 21:11
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