This repository was archived by the owner on Aug 15, 2024. It is now read-only.
Handle SetStreamMetadata idempotently#129
Merged
damianh merged 7 commits intoSQLStreamStore:masterSQLStreamStore/SQLStreamStore:masterfrom May 21, 2018
damianh:idempotent-metadataCopy head branch name to clipboard
Merged
Handle SetStreamMetadata idempotently#129damianh merged 7 commits intoSQLStreamStore:masterSQLStreamStore/SQLStreamStore:masterfrom damianh:idempotent-metadataCopy head branch name to clipboard
damianh merged 7 commits intoSQLStreamStore:masterSQLStreamStore/SQLStreamStore:masterfrom
damianh:idempotent-metadataCopy head branch name to clipboard
Conversation
Member
Author
|
I'm comfortable in declaring this as non-breaking. |
damianh
commented
May 20, 2018
|
|
||
| var metadata = await store.GetStreamMetadata(streamId); | ||
|
|
||
| metadata.MetadataStreamVersion.ShouldBe(0); |
Member
Author
There was a problem hiding this comment.
The primary test. StreamVersion should not have been incremented and stay at 0.
damianh
commented
May 20, 2018
| <GenerateRuntimeConfigurationFiles>true</GenerateRuntimeConfigurationFiles> | ||
| <RootNamespace>SqlStreamStore</RootNamespace> | ||
| <NoWarn>1701;1702;1705;1591</NoWarn> | ||
| <LangVersion>latest</LangVersion> |
Member
Author
There was a problem hiding this comment.
Slipped this in to all projects
damianh
commented
May 20, 2018
| } | ||
|
|
||
| using(var command = new SqlCommand(commandText, connection)) | ||
| using (var command = new SqlCommand(commandText, connection, transaction)) |
Member
Author
There was a problem hiding this comment.
Needed to pass this down because command wouldn't work if in a transaction scope. In other paths, this is null, which is fine.
Contributor
There was a problem hiding this comment.
I'm not completely following - are you saying that particular overload needs to be called even if transaction is null?
Member
Author
There was a problem hiding this comment.
Because this transcation
was getting associated with the connection, if I didn't pass it to this sql command I was getting sql exception.System.InvalidOperationException: BeginExecuteReader requires the command to have a transaction when the connection assigned to the command is in a pending local transaction. The Transaction property of the command has not been initialized.
at System.Data.SqlClient.SqlCommand.ValidateCommand(Boolean async, String method)
at System.Data.SqlClient.SqlCommand.RunExecuteReader(CommandBehavior cmdBehavior, RunBehavior runBehavior, Boolean returnStream, TaskCompletionSource`1 completion, Int32 timeout, Task& task, Boolean asyncWrite, String method)
at System.Data.SqlClient.SqlCommand.BeginExecuteReader(CommandBehavior behavior, AsyncCallback callback, Object stateObject)
at System.Threading.Tasks.TaskFactory`1.FromAsyncImpl[TArg1](Func`4 beginMethod, Func`2 endFunction, Action`1 endAction, TArg1 arg1, Object state, TaskCreationOptions creationOptions)
at System.Threading.Tasks.TaskFactory`1.FromAsync[TArg1](Func`4 beginMethod, Func`2 endMethod, TArg1 arg1, Object state)
at System.Data.SqlClient.SqlCommand.ExecuteReaderAsync(CommandBehavior behavior, CancellationToken cancellationToken)
Member
Author
|
Would like to slip this into |
yreynhout
reviewed
May 21, 2018
| /// <returns> | ||
| /// A deterministically generated GUID. | ||
| /// </returns> | ||
| public Guid Create(IEnumerable<byte> source) |
Contributor
There was a problem hiding this comment.
Very minor, but there's no reason to step into the enumerable world - sticking with byte arrays keeps the translation down to a minimum.
| } | ||
|
|
||
| using(var command = new SqlCommand(commandText, connection)) | ||
| using (var command = new SqlCommand(commandText, connection, transaction)) |
Contributor
There was a problem hiding this comment.
I'm not completely following - are you saying that particular overload needs to be called even if transaction is null?
… be used by all storage implementations.
…essage id. The underlying append operation handles the operation idempotently
… setting stream metadata. Need to pass the existing outer transaction to subsequent sql commands.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
In many cases a developer will lazily set the metadata of a stream. There are scenarios, especially when appending with StreamVersion.Any that they may set the same metadata over and over. This will increase the metadata stream length unnecessarily.
In another case, if users want to handling this idempotently, they have to invoke a read'n'check first. This has subtle race conditions.
This PR:
MetadataMessageIdGeneratorwith a fixed GUID namespace.