Skip to content
This repository has been archived by the owner on Nov 14, 2024. It is now read-only.

TransactionManager#startTransactions API. #4799

Merged
merged 10 commits into from
May 27, 2020
Merged

Conversation

jkozlowski
Copy link
Contributor

@jkozlowski jkozlowski commented May 26, 2020

Goals (and why):

  • Return OpenTransaction that knows how to close the transaction wrt. open resources.
  • Remove start/finish pair of methods that is very C-like.
  • Hide the immutable timestamp lock token.
  • OpenTransactions can contain any lockwatch-related methods, so they do not have to be exposed outside of this flow, as it doesn't make sense otherwise.

Implementation Description (bullets):

Testing (What was existing testing like? What have you done to improve it?):

Concerns (what feedback would you like?):

  • It feels like an improvement, but there might be something even more drastic.

Where should we start reviewing?:

Priority (whenever / two weeks / yesterday):

@changelog-app
Copy link

changelog-app bot commented May 26, 2020

Generate changelog in changelog/@unreleased

Type

  • Feature
  • Improvement
  • Fix
  • Break
  • Deprecation
  • Manual task
  • Migration

Description

Add TransactionManager#startTransactions API that replaces previous #setupRunTaskBatchWithConditionThrowOnConflict and #finishRunTaskWithLockThrowOnConflict.

Check the box to generate changelog(s)

  • Generate changelog entry

Copy link
Contributor

@jeremyk-91 jeremyk-91 left a comment

Choose a reason for hiding this comment

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

Preliminary feedback (only scanned, didn't review impl in depth)

OpenTransaction

  • Broadly makes sense. In general no reason to expose the immutable timestamp thing
  • Just make sure to check this won’t actually break anyone; backup services and migration lib are probably going to touch immutable timestamps in some way, so I’d imagine they’d be higher risk. (Though they might not go through TMs, in which case they’re fine)

OpenTransactions plural

  • I’d interpret OpenTransactions as List + maybe some derived things, including LockWatch stuff there seems a bit unexpected. (In a good number of contexts (large internal product, embedded) we won’t have and won’t use the Events.)
  • Yet we certainly do want this structure. Maybe if we named it StartTransactionsResponse I’d find this to be less conflicting with my intuition?

@jkozlowski jkozlowski force-pushed the start-transactions-api branch from 8844aa7 to 8f4cf17 Compare May 27, 2020 14:17
Copy link
Contributor

@jeremyk-91 jeremyk-91 left a comment

Choose a reason for hiding this comment

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

For the most part this makes sense. Some minor questions

Copy link
Contributor

@jeremyk-91 jeremyk-91 left a comment

Choose a reason for hiding this comment

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

👍

@bulldozer-bot bulldozer-bot bot merged commit a513fd2 into develop May 27, 2020
@bulldozer-bot bulldozer-bot bot deleted the start-transactions-api branch May 27, 2020 21:04
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants