Skip to content

[WIP]Adds a new pipeline stage in order to be able to implement UoW on top of SynchronizedStorageSession.#4490

Closed
SzymonPobiega wants to merge 1 commit into
developfrom
uow-on-top-of-storage-session
Closed

[WIP]Adds a new pipeline stage in order to be able to implement UoW on top of SynchronizedStorageSession.#4490
SzymonPobiega wants to merge 1 commit into
developfrom
uow-on-top-of-storage-session

Conversation

@SzymonPobiega

@SzymonPobiega SzymonPobiega commented Feb 20, 2017

Copy link
Copy Markdown
Member

Splits the LoadHandlersConnector into two and inserts IUnitOfWorkContext stage between them:

  • SynchronizedSessionConnector only manages the synchronized storage session
  • LoadHandlersConnector only loads the handlers

Related customer cases:

TODO:

@SzymonPobiega SzymonPobiega self-assigned this Feb 20, 2017
/// <summary>
/// Tells if the message has been handled.
/// </summary>
bool MessageHandled { get; set; }

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need this here?

Pretty sure it was added as a hack to support the callbacks flagging a message as handled

https://github.com/Particular/NServiceBus.Callbacks/blob/7d69f94979e7dc82c8527d7cbc0a0d95a863a7e0/src/NServiceBus.Callbacks/Reply/RequestResponseInvocationForMessagesBehavior.cs#L31

to avoid us throwing a "message wasn't handled" exception.

I don't think we need that hack here?

/// <summary>
/// Metadata for the incoming message.
/// </summary>
MessageMetadata MessageMetadata { get; }

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd say we should go with message type only

https://github.com/Particular/NServiceBus/blob/develop/src/NServiceBus.Core/Unicast/Messages/MessageMetadata.cs#L34

to avoid exposing the message meta data further (I think we can 🔥 it going forwards)

@andreasohlund

Copy link
Copy Markdown
Member

If we go ahead with this we should change https://docs.particular.net/samples/pipeline/unit-of-work/ accordingly

@SzymonPobiega

Copy link
Copy Markdown
Member Author

@andreasohlund yes, that's the sample check box in the plan of attach. I'll make it clear.

@andreasohlund andreasohlund changed the title Adds a new pipeline stage in order to be able to implement UoW on top of SynchronizedStorageSession. [WIP]Adds a new pipeline stage in order to be able to implement UoW on top of SynchronizedStorageSession. Feb 21, 2017
@andreasohlund

Copy link
Copy Markdown
Member

Marking this as WIP after a call with @SzymonPobiega and @timbussmann .

We think that the short term fix is to expose these hooks in the native persister instead (NH + SqlPersister).

@SzymonPobiega will do a NH spike.

We still believe that a UoW stage is good but it can likely wait to v7

@timbussmann

Copy link
Copy Markdown
Contributor

looking forward for the nh spike. To capture my thoughts which align with the decision made as described by @andreasohlund:

  • the "unit of work" stage seems to make sense. Especially considering the fact that the unit of work api currently yields no benefit over just providing a custom behavior. So that seems to be a good way forward in improving the unit of work "API".
  • Similar to RavenDB, it should be a persistences responsibility to provide access to it's data store from within handlers. Not sure where a unit of work in that regards would be required, as the lifetime of the session would already be managed by the persistence package.

@SzymonPobiega

Copy link
Copy Markdown
Member Author

Here's a NH PR Particular/NServiceBus.NHibernate#266

@andreasohlund

Copy link
Copy Markdown
Member

Closed in favor of #4493

@andreasohlund andreasohlund deleted the uow-on-top-of-storage-session branch February 22, 2017 09:47
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.

3 participants