-
Notifications
You must be signed in to change notification settings - Fork 12
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
Add support for adopting an existing connection/transaction #273
Conversation
d50cac1
to
1b56110
Compare
85421af
to
f3f14af
Compare
f3f14af
to
4d95e6e
Compare
@@ -586,6 +644,8 @@ void AddCommandTrace(string commandText) | |||
if (commandTrace.Count <= 200) | |||
commandTrace.Add(DateTime.Now.ToString("s") + " " + commandText); | |||
} | |||
|
|||
customCommandTrace?.Invoke(commandText); |
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.
When this is provided, we could disable the default command tracing behaviour and leave it up to the consumer only. However, other things that are tied to this still exist like RelationalTransactionRegistry
, and there's no guarantee the consumer doesn't still want that stuff. So, I think it's safest to just leave it be.
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.
An alternative could be to make it the default implementation. That might get tricky to get parity with the current locking behaviour though. It looks like we hold a collection of traces for the lifetime of this transaction and also use it as a locking mechanism?
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.
It looks like we hold a collection of traces for the lifetime of this transaction and also use it as a locking mechanism?
Yep 👍 and Nevermore relies on that collection elsewhere like in RelationalTransactionRegistry
, so turning it off when we provide a custom implementation would stop that from working. It may be solvable but I think it's best to keep it an additional thing right now.
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.
The big-picture design and implementation looks sound.
I've offered some suggestions on ways you might improve/simplify some things, but I'm happy if you choose to disagree on them.
Will leave approval to the Normalisation sub-group
source/Nevermore.IntegrationTests/Advanced/UseExistingTransactionFixture.cs
Outdated
Show resolved
Hide resolved
source/Nevermore.IntegrationTests/Advanced/UseExistingTransactionFixture.cs
Outdated
Show resolved
Hide resolved
source/Nevermore.IntegrationTests/Advanced/UseExistingTransactionFixture.cs
Outdated
Show resolved
Hide resolved
source/Nevermore.IntegrationTests/Advanced/UseExistingTransactionFixture.cs
Outdated
Show resolved
Hide resolved
217d133
to
d48cd38
Compare
((Action)(() => sqlTransaction.Commit())).Should().NotThrow(); | ||
sqlConnection.State.Should().Be(ConnectionState.Open); | ||
} | ||
} |
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.
Some good edge cases covered 👍
Would it be possible to write a test case for the happy path for an externally owned connection?
e.g. start a non-owned transaction, do some work, commit, verify changes?
RetriableOperation operationsToRetry, | ||
IRelationalStoreConfiguration configuration, | ||
IKeyAllocator keyAllocator, | ||
string name = null | ||
) : base(store, registry, operationsToRetry, configuration, name) | ||
Action<string>? customCommandTrace = null, |
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.
is this unique to each ctor invocation, or could it be on the config object?
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.
In theory you could put it on the config object and for some simple use cases that might be sufficient, but in practice it's so much more convenient if you can directly associate the tracing with the specific transaction that the trace is coming from by just binding them together like this.
Another nice property from doing it this way is it's optional per Nevermore transaction; for externally managed transactions this is really useful, but if you're not adopting an existing transaction then it's less useful, so in a codebase that does a mix of both you might only want custom tracing in some cases and can just pass a null
in the others.
source/Nevermore.IntegrationTests/Advanced/UseExistingTransactionFixture.cs
Outdated
Show resolved
Hide resolved
|
||
var writeTransaction = (WriteTransaction) Store.CreateWriteTransactionFromExistingConnectionAndTransaction(sqlConnection, sqlTransaction); | ||
|
||
using (new AssertionScope()) |
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.
I'm surprised the IDE didn't complain at you and cause you to change this into using var scope = new AssertionScope();
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.
It only suggests that if you're already giving it an alias i.e. using (var x = new AssertionScope())
🙂
0ef150a
to
42b9a20
Compare
Background
This allows Nevermore to work with an existing connection/transaction pair rather than always creating and owning its own. This allows multiple Nevermore transactions to be used as part of a single SQL transaction, or sharing a single transaction between multiple ORMs, or simply enabling the ownership of the connection/transaction to remain with the consumer.
This is similar to EF's
UseTransaction
: https://learn.microsoft.com/en-us/ef/core/saving/transactions#share-connection-and-transactionIf an external connection is provided, the consumer is responsible for commit/dispose of the connection/transaction.
Notes
This is a breaking change as there are changes to public APIs. Nevermore's major version will be incremented as part of this change.
Notably, there are new arguments to the
ReadTransaction
andWriteTransaction
constructors as well as whole new constructor overloads, and theIRelationalStore
interface now has two new methodsCreateReadTransactionFromExistingConnectionAndTransaction
CreateWriteTransactionFromExistingConnectionAndTransaction