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

Allows use of externally owned transactions for writes #282

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

jimmyp
Copy link

@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
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

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
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

Copy link

Copy link
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
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.

None yet

4 participants