-
Notifications
You must be signed in to change notification settings - Fork 100
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
breaking: refactor(transaction): split logic into new classes #506
breaking: refactor(transaction): split logic into new classes #506
Conversation
Previously all transaction logic was housed in a single class. This refactor breaks the logic up into multiple classes: Snapshot - read only transactions Transaction - read/write transactions PartitionedDml - dml capable transaction
You mention this will bring us more in line with other client libs. Which other languages are you referencing in particular? |
@crwilcox I think all of them. AFAIK we were the other lib to treat snapshots and r/w transactions the same way. You could call |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Just make sure to do a release relatively soon after with good release notes :)
Yeah, I'm hoping @crwilcox has time to take a run through :) |
This is a real doozy. It is just large enough it is hard to really accurately review. While I looked it over I am not confident I will have found anything useful :( That said, here are some thoughts.
|
Yeah, thats a fail on my part, I should probably have used a more descriptive commit message. The gist is that this is a refactor to Transactions as a whole with the biggest piece being the split. I was already in there touching that code so I figured I might as well improve the timestamp bound support.
Yes, they were incorrect. I hand wrote that file to provide types for the gapics and help cut back on duplicate interfaces declared across the handwritten files. Sometimes it isn't entirely clear from the documentation whether or not a property is optional (at least not to me) and there isn't any official TypeScript support for gapics (yet). |
Thanks for the response @callmehiphop , re: my first bit about a sample of read/write, I suppose I was asking if you could make sure your notes for the change are reflected? Your current description for this PR seems to lack that so I wanted to be sure we help users migrate. |
Previously all transaction logic was housed in a single class. This refactor breaks the logic up into multiple classes:
Snapshot
- read only transactionsTransaction
- read/write transactionsPartitionedDml
- partitioned dml capable transactionAdditionally it decouples the
runTransaction
logic from the Transaction class itself by introducing a new classTransactionRunner
.You might be asking yourself, "But why, Dave?". This will (IMO) clean up the code base and help us align more closely with the other client libs. It seems we did some stuff differently without good reason.
While I admit the diff on this PR is kind of insane, the breaking changes are actually pretty minimal, as highlighted below.
Running a read only transaction
Snapshots should never give back an
ABORTED
error, so really there is no reason to run them viarunTransaction
which containsABORTED
/UNKNOWN
retry logic. It might also be worth noting that we are the only client library that ever has allowed snapshots to be ran this way.Before
After
Timestamp bounds now expect milliseconds (instead of seconds)
This one seems kind of obvious, not sure why we did it this way, but these should allow for as much precision as we can provide.
Before
After
Transaction#end
is now synchronousThis mainly applies to read only transactions, as
commit
androllback
have always calledend
under the hood.Before
After
Session#beginTransaction
was removed in favor of transaction factoriesThis is more of an implementation detail change. Users typically won't need to use this API because the lib currently tries to hide sessions as much as possible. They are still accessible, but mostly discouraged.
Before
After