GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — Handle SetStreamMetadata idempotently by damianh · Pull Request #129 · SQLStreamStore/SQLStreamStore

This repository was archived by the owner on Aug 15, 2024. It is now read-only.

Conversation

damianh commented May 20, 2018

Copy link

Copy Markdown

Member

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:

Adds a deterministic guid generator and tests
Add a MetadataMessageIdGenerator with a fixed GUID namespace.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

damianh self-assigned this

May 20, 2018

damianh added the enhancement label

May 20, 2018

damianh commented May 20, 2018

Copy link

Copy Markdown

Member Author

I'm comfortable in declaring this as non-breaking.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

damianh changed the title ~~[WIP] Handle SetStreamMetadata idempotently~~ Handle SetStreamMetadata idempotently

May 20, 2018

damianh commented

May 20, 2018

View reviewed changes

Comment thread

src/SqlStreamStore.AcceptanceTests/SqlStreamStoreAcceptanceTests.StreamMetadata.cs


		var metadata = await store.GetStreamMetadata(streamId);

		metadata.MetadataStreamVersion.ShouldBe(0);

damianh May 20, 2018

Copy link

Copy Markdown

Member Author

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 primary test. StreamVersion should not have been incremented and stay at 0.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

damianh commented

May 20, 2018

View reviewed changes

Comment thread

src/SqlStreamStore.MsSql.Tests/SqlStreamStore.MsSql.Tests.csproj

		<GenerateRuntimeConfigurationFiles>true</GenerateRuntimeConfigurationFiles>
		<RootNamespace>SqlStreamStore</RootNamespace>
		<NoWarn>1701;1702;1705;1591</NoWarn>
		<LangVersion>latest</LangVersion>

damianh May 20, 2018

Copy link

Copy Markdown

Member Author

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.

Slipped this in to all projects

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

damianh commented

May 20, 2018

View reviewed changes

Comment thread

src/SqlStreamStore.MsSql/MsSqlStreamStore.ReadStream.cs

		}

		using(var command = new SqlCommand(commandText, connection))
		using (var command = new SqlCommand(commandText, connection, transaction))

damianh May 20, 2018

Copy link

Copy Markdown

Member Author

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.

Needed to pass this down because command wouldn't work if in a transaction scope. In other paths, this is null, which is fine.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

yreynhout May 21, 2018

Copy link

Copy Markdown

Contributor

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 not completely following - are you saying that particular overload needs to be called even if transaction is null?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

damianh May 21, 2018

Copy link

Copy Markdown

Member Author

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.

Because this transcation

SQLStreamStore/src/SqlStreamStore.MsSql/MsSqlStreamStore.StreamMetadata.cs

Line 61 in 4acea41

using(var transaction = connection.BeginTransaction())

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)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

damianh requested review from thefringeninja and yreynhout

May 20, 2018 18:51

damianh added this to the v1.1.2 milestone

May 20, 2018

damianh commented May 20, 2018

Copy link

Copy Markdown

Member Author

Would like to slip this into v1.1.2

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

damianh mentioned this pull request

May 20, 2018

Allows setting of $deleted stream's metadata #131

Merged

damianh force-pushed the master branch from 3b2614a to 110929c Compare

May 20, 2018 20:54

yreynhout reviewed

May 21, 2018

View reviewed changes

Comment thread

src/SqlStreamStore/Infrastructure/DeterministicGuidGenerator.cs Outdated

		/// <returns>
		/// A deterministically generated GUID.
		/// </returns>
		public Guid Create(IEnumerable<byte> source)

yreynhout May 21, 2018

Copy link

Copy Markdown

Contributor

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.

Very minor, but there's no reason to step into the enumerable world - sticking with byte arrays keeps the translation down to a minimum.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

src/SqlStreamStore.MsSql/MsSqlStreamStore.ReadStream.cs

		}

		using(var command = new SqlCommand(commandText, connection))
		using (var command = new SqlCommand(commandText, connection, transaction))

yreynhout May 21, 2018

Copy link

Copy Markdown

Contributor

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 not completely following - are you saying that particular overload needs to be called even if transaction is null?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

damianh added 7 commits

May 21, 2018 13:24


          Add a deterministic guid generator and tests

070147e


          Set LangVersion to latest in all projects

a16c282


          Add failing test for handling SetStreamMetadata idempotently

d205bbb


          The metadata message id generator with a fixed guid namespace. Should…

30ad68f

… be used by all storage implementations.


          InMemoryStreamStore: use MetadataMessageIdGenerator to generate the m…

276f90d

…essage id. The underlying append operation handles the operation idempotently


          MsSql: use MetadataMessageIdGenerator to generate the message id when…

6c75a1a

… setting stream metadata. Need to pass the existing outer transaction to subsequent sql commands.


          Just take an array

d598533

damianh merged commit b54cac7 into SQLStreamStore:master

May 21, 2018

damianh deleted the idempotent-metadata branch

May 21, 2018 11:35

Reviewers

thefringeninja

Awaiting requested review from thefringeninja

1 more reviewer

yreynhout

yreynhout left review comments

Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

damianh

Labels

enhancement

Projects

None yet

Milestone

v1.1.2

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

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.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.