Minimal TOC #4860
Minimal TOC #4860
Conversation
Minimal transfer of coordintation support for transactions. - TransactionCommitter facet - Changed to commit logic to commit only upon successfull remot service call. - Minima golden path tests
|
@dotnet-bot test functional please |
| /// <summary> | ||
| /// An identifier of the current transaction. Can be used for tracing and debugging. | ||
| /// </summary> | ||
| string CurrentTransactionId { get; } |
There was a problem hiding this comment.
Attempting to keep surface clean. The transactionId, even it's existence, is an implementation detail for ITransactionalState.
There was a problem hiding this comment.
I disagree. It's not an implementation detail. It is part of the public transaction API. I find it useful and use it in my tests and my workload.
There was a problem hiding this comment.
If needed, call should be added to the TransactionContext, not the facet.
| foreach (var actual in actualValues) | ||
| { | ||
| Assert.Equal(expected, actual); | ||
| } |
There was a problem hiding this comment.
can we add a check somewhere in this test to check the remote service has been called and succeed, since otherwise we are not really testing toc
There was a problem hiding this comment.
This has been verified manually via logging, as the test service only logs at this time. When we build out a more complete test service, we can add verification that service was called. I'll add a comment in the code to add this in future a pass.
There was a problem hiding this comment.
hmm verify via logging is far less then ideal. but I'm ok with fixing it in future pass
| [InlineData(TransactionTestConstants.SingleStateTransactionalGrain, TransactionTestConstants.MaxCoordinatedTransactions)] | ||
| [InlineData(TransactionTestConstants.DoubleStateTransactionalGrain, TransactionTestConstants.MaxCoordinatedTransactions / 2)] | ||
| [InlineData(TransactionTestConstants.MaxStateTransactionalGrain, 1)] | ||
| public virtual async Task MultiGrainWriteTransaction(string grainStates, int grainCount) |
There was a problem hiding this comment.
can we also add a simple failure test case? if the remote service failed to commit, then transaction abort. I think minimal testing for toc should include at least one successful path testing and one failure path testing.
There was a problem hiding this comment.
At this time, we've committed only to the public surface and minimal golden path (which does not include failure testing). We'll be adding more testing to this in future passes.
|
@dotnet-bot test functional please |
|
do you want to merge this, or you are still waiting for other people to participate? |
Minimal transfer of coordintation support for transactions.