Skip to content

Allows use of externally owned transactions for writes#282

Draft
jimmyp wants to merge 4 commits into
masterfrom
jp/commiting-non-owned-transactions
Draft

Allows use of externally owned transactions for writes#282
jimmyp wants to merge 4 commits into
masterfrom
jp/commiting-non-owned-transactions

Conversation

@jimmyp
Copy link
Copy Markdown

@jimmyp jimmyp commented Feb 22, 2024

Nevermore already had the concept of accepting an external transaction to use for reads. This PR extends that behaviour to writes.

Commit() was renamed to TryCommit() to convey that it can now throw an exception if the externally provided transaction has already been committed elsewhere. This caused a lot of noise in the tests but I think the rename is valuable otherwise this would be a breaking change.

[sc-71481]

@jimmyp jimmyp marked this pull request as ready for review February 22, 2024 04:22
@jimmyp jimmyp requested a review from a team as a code owner February 22, 2024 04:22
@jimmyp jimmyp marked this pull request as draft February 22, 2024 04:33
@jimmyp jimmyp changed the title allows commiting of non owned transactions Allows use of externally owned transactions for writes Feb 29, 2024
@michaelongso
Copy link
Copy Markdown
Contributor

michaelongso commented Mar 1, 2024

Tagged Cloud PT, https://octopusdeploy.slack.com/archives/C01Q95KPRM4/p1709254205324879

No impact for Cloud PT as they have removed Nevermore from Cloud Portal

@gb-8
Copy link
Copy Markdown

gb-8 commented Mar 1, 2024

Commit() was renamed to TryCommit() to convey that it can now throw an exception if the externally provided transaction has already been committed elsewhere.

I'm not following this. Previously Commit() would throw an exception if the SQL transaction was externally owned. Now it just silently does nothing. That is a breaking change.

The new TryCommit() will throw for externally owned SQL transactions, like the old Commit() method - it won't even get as far as finding out if the externally owned SQL transaction has already been committed.

I'm confused.

@jimmyp
Copy link
Copy Markdown
Author

jimmyp commented Mar 1, 2024

Previously Commit() would throw an exception if the SQL transaction was externally owned.

You are totally right, I got myself confused while trying to use the MS convention on TryXXX methods... To not make this a breaking change I'll have to not use this convention

@shortcut-integration
Copy link
Copy Markdown

Comment thread source/Nevermore/Advanced/WriteTransaction.cs
Comment thread source/Nevermore/Advanced/WriteTransaction.cs
Copy link
Copy Markdown
Contributor

@adam-mccoy adam-mccoy left a comment

Choose a reason for hiding this comment

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

Blocking this one for further discussion.

Comment on lines +8 to +9
void CommitIfOwned();
Task CommitIfOwnedAsync(CancellationToken cancellationToken = default);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Per the feedback on https://github.com/OctopusDeploy/OctopusDeploy/pull/23120, we'd rather not introduce a different way to commit a Nevermore transaction. Rather, the consumer (Octopus Server) should be aware of whether the SQL transaction is externally owned and simply not call Commit() on the Nevermore transaction in that case. Happy to assist with ways to solve that.

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.

4 participants