Particular / NServiceBus

Build, version, and monitor better microservices with the most powerful service platform for .NET
https://particular.net/nservicebus/
Other
2.07k stars 650 forks source link

Obsolete sending and publishing batches of messages together in a single call #1346

Closed johnsimons closed 10 years ago

johnsimons commented 11 years ago

Each of the messaging APIs that involve multiple messages on IBus have been removed. Below is the table on how to replace these.

Old Method Replacement Method
Publish(T[] messages); Publish(T message);
SendLocal(object[] messages); SendLocal(object message);
Send(object[] messages); Send(object message);
Send(string destination, object[] messages); Send(string destination, object message);
Send(Address address, object[] messages); Send(Address address, object message);
Send(string destination, string correlationId, object[] messages); Send(string destination, string correlationId, object message);
SendToSites(IEnumerable siteKeys, object[] messages); SendToSites(IEnumerable siteKeys, object message);
Defer(TimeSpan delay, object[] messages); Defer(TimeSpan delay, object message);
Defer(DateTime processAt, object[] messages); Defer(DateTime processAt, object message);
Reply(object[] messages); Reply(object message);

We have had a poll on this and the results are in favour to get rid of this functionality.

Poll results:

Choices Votes %
Not at all - I don't use it 35 61
Very little - I use it in a couple of places, but it wouldn't be difficult to change them 19 33
Impacted - I'm using it quite a bit so there would be a fair bit of cleanup to do 1 1
It's the end of the world - I really depend on this feature and if it weren't there, I'd have to evaluate whether to keep using NServiceBus 2 3

It was then decided to leave this feature as it is for v4, and instead change it for v5.

@udidahan last comment is:

In v5.0 we’ll provide a session instead.

I assume this means something like how ASP.NET has a SessionId header?

Should we make a spike on how to solve this issue using a session?

andreasohlund commented 11 years ago

+1

Sent from my iPhone

On 25 jul 2013, at 04:22, John Simons notifications@github.com wrote:

We have had a poll on this and the results are in favor to get rid of this functionality.

Poll results:

Choices Votes % Not at all - I don't use it 35 61 Very little - I use it in a couple of places, but it wouldn't be difficult to change them 19 33 Impacted - I'm using it quite a bit so there would be a fair bit of cleanup to do 1 1 It's the end of the world - I really depend on this feature and if it weren't there, I'd have to evaluate whether to keep using NServiceBus 2 3 It was then decided to leave this feature as it is for v4, and instead change it for v5.

@udidahan last comment is:

In v5.0 we’ll provide a session instead.

I assume this means something like how ASP.NET has a SessionId header?

Should we make a spike on how to solve this issue using a session?

— Reply to this email directly or view it on GitHub.

udidahan commented 10 years ago

OK – but make sure you’re proactive about communicating about this change.

danielmarbach commented 10 years ago

Why not make it a pluggable feature with extensions, custom serialzer etc. which is not enabled by default apart from v5?

udidahan commented 10 years ago

High complexity cost, low benefit.

johnsimons commented 10 years ago

So just we all on the same page.

Are we saying that:

  1. do a spike and try to figure out a workaround
  2. drop this feature and don't support batch sending within same transaction

What is it?

udidahan commented 10 years ago

I was speaking to all the pluggability. I do think we need to have a feature like:

using (var batch = bus.StartBatch()) { bus.Send(a); bus.Send(b); batch.Complete(); /* do we really need a complete here? */ }

lcorneliussen commented 10 years ago

(don't know yet if it was a good idea to follow the repo :))

Some thoughts:

danielmarbach commented 10 years ago

I think with all the new transports we should not handle batching in the upper layers but in the lower layers (meaning in the transport itself). I.ex. Activemq allows to group multiple messages together

Am 30.07.2013 um 12:21 schrieb Lars Corneliussen notifications@github.com:

(don't know yet if it was a good idea to follow the repo :))

Some thoughts:

Message headers are (still?) kind-of broken in multi-message transport messages. (Registers and picks headers from first message) - this should be fixed by an extra envelope pr. message... @udidahan what is a batch? what if one tries to send messages of different types? or publish or defer single messages...? if it is about batching the transport, then it would be enough to collect messages pr. address and send them batched. (two sends + a publish to the same queue would end in one transport message); if it only is for replacing the current Send(object[]), the address would have to be part of StartBatch... — Reply to this email directly or view it on GitHub.

johnsimons commented 10 years ago

Message headers are (still?) kind-of broken in multi-message transport messages. (Registers and picks headers from first message) - this should be fixed by an extra envelope pr. message...

Agree, this is really the issue that we are trying to address here, unfortunately introducing an extra envelope per logical message is a big non-backwards breaking change! but I really don't see a workaround :neutral_face:

I also think that we need to address the confusion that exists with Send(object[]), and I like the idea of maybe renaming this method to be more explicit, something like SendAsBatch(object[])

chrisbednarski commented 10 years ago

(two sends + a publish to the same queue would end in one transport message) ??? Is this a good idea in the general case? What about having independent retries for each message in a batch?

Isn't the only reason to use a batch to roll back message groups as a whole?

udidahan commented 10 years ago

Forget about my "using ()" suggestion - it just makes things more difficult. Renaming to SendAsBatch is reasonable, but still leaves us with the header issue Lars mentioned.

I remember Johann Gustafsson mentioning he really needed this feature - let's talk to him to understand his specific scenario and then maybe we'll be able to make a more informed decision.

johannesg commented 10 years ago

Yes, I remember saying something about it but i probably misunderstood/overreacted. We use batching sparingly, and mostly as optimization. This could probably be refactored to put the batch inside the message itself which is probably preferable and more explicit.

If you are to keep it, I'm in favor of refactoring it to a separate method, ie. SendAsBatch. Having Send(object[]) and Send(object) is certainly confusing.

udidahan commented 10 years ago

OK, so that settles it. We're removing Send(object[]), Publish(object[]), and any other batch APIs.

andreasohlund commented 10 years ago

+1

I'd also like to switch over to the "opts" idea that @lcorneliussen suggested here:

https://github.com/NServiceBus/NServiceBus/pull/1347

Since that really makes the IBus api more terse and allows us to evolve it more easily in the future. Not to mention that is also solve the whole "bus.Defer" discussion.

On Mon, Aug 5, 2013 at 3:03 PM, Udi Dahan notifications@github.com wrote:

OK, so that settles it. We're removing Send(object[]), Publish(object[]), and any other batch APIs.

— Reply to this email directly or view it on GitHubhttps://github.com/NServiceBus/NServiceBus/issues/1346#issuecomment-22104505 .

http://andreasohlund.net http://twitter.com/andreasohlund

johnsimons commented 10 years ago

I think we should do the renames for 4.1, thoughts?

andreasohlund commented 10 years ago

No objections

Sent from my iPhone

On 6 aug 2013, at 03:27, John Simons notifications@github.com wrote:

I think we should do the renames for 4.1, thoughts?

— Reply to this email directly or view it on GitHub.

pawelpabich commented 10 years ago

Hey,

So if I understand correctly there will be no way of sending multiple messages at once?

johnsimons commented 10 years ago

@pawelpabich, There won't be a away to send multiple logical messages as one physical message, does this make sense?

SimonCropp commented 10 years ago

@pawelpabich as in

each message will be processed in its own transaction

instead of

multiple messages being processed inside a single transaction

pawelpabich commented 10 years ago

ok, thanks

charlessolar commented 10 years ago

Out of curiosity, does CompletionResult's Messages property still need to be an array?

andreasohlund commented 10 years ago

No it won't have to be, we'll replace it with a .Message instead:

https://github.com/Particular/NServiceBus/issues/2073

Thanks for bringing this to out attention!

On Sun, Apr 27, 2014 at 10:25 AM, Charles Solar notifications@github.comwrote:

Out of curiosity, does CompletionResult's Messages property still need to be an array?

Reply to this email directly or view it on GitHubhttps://github.com/Particular/NServiceBus/issues/1346#issuecomment-41491037 .

SimonCropp commented 10 years ago

fixed in https://github.com/Particular/NServiceBus/commit/34ff7e6cd4f4177a8d249a2d5b6687540d370b48

iskandersierra commented 9 years ago

@udidahan or @SimonCropp: I'm a little bit too late into this discussion but I'm going to ask a situation I have while implementing DDD with EventSourcing and using NServiceBus as the service bus. One of my command handlers receive a single command in my domain object e.g.: CreateContact { Id: "123", Title: "Anyone"} and from there I want to emit two events ContactCreated {} and ContactTitleUpdated {Title: "Anyone"} but I would like to process them in a transaction on my EventHandler (projection). How can I do that if messages wont come in a single batch?

nohros commented 9 years ago

Two distinct messages should be processed in two distinct transactions. If they represent a single concept they it should be a represented by a single message. If they represent distinct concepts that are dependant you could use a Saga to process them.