Skip to content

DynamoDB transaction log#4056

Merged
jason-bragg merged 6 commits intodotnet:masterfrom
PerBlue:dynamodb_transaction_log
Mar 26, 2018
Merged

DynamoDB transaction log#4056
jason-bragg merged 6 commits intodotnet:masterfrom
PerBlue:dynamodb_transaction_log

Conversation

@jsteinich
Copy link
Contributor

Ported Azure transaction log to dynamodb.
Made a small api change to DynamoDBStorage in order to get continuation information from queries.

@ReubenBond
Copy link
Member

@galvesribeiro please take a look

@galvesribeiro
Copy link
Member

Thanks @jsteinich. I'll have a look later tonight when I got home.

However, before we merge it, perhaps would be good to wait for #4042 to get merged as there are changes on how the provider subsystem works and some refactory I did on the AWS providers so you can rebase this provider on top of master.

@jason-bragg
Copy link
Contributor

I definitely don't want to discourage extending the range of platforms supported by transactions, but the transaction log storage abstractions are quite immature at this time, and have known issues. If users are actively working with transactions in services using DynamoDb, then we should vet and support this addition, but if this is being ported to DynamoDb simply to have coverage, I'd advocate holding off until the abstractions are more mature.

@sergeybykov sergeybykov added this to the Triage milestone Feb 21, 2018
@jsteinich
Copy link
Contributor Author

I did need to do a small amount of legacy config in the tests, so waiting for #4042 would allow for cleaning that up.

So far the project I'm working on has been local to my computer so I've just been using the in memory transaction log. I'm planning on getting an environment set up in AWS in the next couple of weeks that I would like to closely match the eventual production setup. The transaction log was just something on the list to meeting that goal. It isn't crucial for us to have this right away, but we are planning to make heavy use of transactions and expect to go through some of the growing pains.

@jason-bragg
Copy link
Contributor

@jsteinich, Thanks for the contribution, and if you're planning on using this, then we'll definitely accept the PR (once reviewed) and maintain it, including updating it as the abstractions change. Just wanted to make sure the extra work equated to real value in the world, especially given the beta state of the feature.

Copy link
Contributor

@jason-bragg jason-bragg 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 not qualified to vet the DynamoDB specific logic, but the pattern looks consistent with the existing transaction logging and extension library patterns.

It also includes some basic testing which I presume the author has run. It's important for maintainers to exercise this because we don't have DynamoDb testing as part of our automated (CI, or nightly) test runs.

The PR needs rebased. It would be good for others in the community with DynamoDB experience to vet this, but as it does not introduce much risk to the existing transaction work, I advocate we merge this PR after the rebase, barring others finding issues in it.

Copy link
Member

@galvesribeiro galvesribeiro left a comment

Choose a reason for hiding this comment

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

LGTM

@galvesribeiro
Copy link
Member

@jason-bragg

It also includes some basic testing which I presume the author has run. It's important for maintainers to exercise this because we don't have DynamoDb testing as part of our automated (CI, or nightly) test runs.

Once we have the testhost xplat, that will not be an issue. I'll do myself the port of the tests to grab the dependencies required for those providers so you would be safe to enable then in the functional run.

@jsteinich jsteinich force-pushed the dynamodb_transaction_log branch from a60d5ef to 9a7fb9c Compare March 22, 2018 21:35
@jsteinich
Copy link
Contributor Author

Looks like the test failed for something unrelated.

@galvesribeiro
Copy link
Member

@dotnet-bot test functional please

@jason-bragg jason-bragg merged commit 2274e0d into dotnet:master Mar 26, 2018
@sergeybykov
Copy link
Contributor

Thank you, @jsteinich!

@sergeybykov
Copy link
Contributor

@jsteinich I opened #4906 for migration of the provider against the new interface.

@github-actions github-actions bot locked and limited conversation to collaborators Dec 8, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants