search

Particular / NServiceBus

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

Create/update subscriptions in bulk #4632

Open SeanFeldman opened 7 years ago

SeanFeldman commented 7 years ago

Current Subscription API caters for individual subscriptions (create/delete).

During startup, this information could be passed in as a collection to allow transport to optimize the startup time by leveraging smaller number of management operations.

Original (user) request: https://github.com/Particular/NServiceBus.AzureServiceBus/issues/522

timbussmann commented 7 years ago

so essentially a messageSession.Subscribe(eventType1, eventType2, eventType3, ...) is what would improve your situation?

But it seems that it would be a cleaner approach to follow up on the "batchable sends" ideas for the message session in general, as this the above api would allow batching for subscriptions but not for sends.

(would a messageSession.Send(message1, message2, message3) be the simplest way to implement the batching instead of considering session lifetimes?) @andreasohlund @danielmarbach

SeanFeldman commented 7 years ago

Not quite. What it would mean is to have an addition to IManageSubscriptions that has a Subscribe(List<Type> events, ContextBag context) that is invoked when endpoint is bootstrapped and events are scanned. Doing it at the time of when messages are send (i.e. messageSession.Send(message1, message2, message3)) is too late.

andreasohlund commented 7 years ago

We have talked about passing the list of subscribers to the transport at .Init time. That would be better imo since allows the transport to abort startup should something go wrong when the subscriptions are setup? On Fri, 21 Apr 2017 at 16:20, Sean Feldman notifications@github.com wrote:

Not quite. What it would mean is to have an addition to IManageSubscriptions that has a Subscribe(List events, ContextBag context) that is invoked when endpoint is bootstrapped and events are scanned. Doing it at the time of when messages are send (i.e. messageSession.Send(message1, message2, message3)) is too late.

— You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub https://github.com/Particular/NServiceBus/issues/4632#issuecomment-296203947, or mute the thread https://github.com/notifications/unsubscribe-auth/AAHoZCv0_02BMWo8C9QGDRwj0p4TCvNCks5ryLtKgaJpZM4NDOZz .

timbussmann commented 7 years ago

so the core currently only subscribes via the AutoSubscribe startup task which is done when the endpoint is essentially started but before it starts consuming messages. Passing subscribers earlier would probably be quite a change and only works with native pub/sub transports and not with message driven pub/sub (as it requires a started transport by it's nature) so that sounds like a much larger change compared to my proposal?

@SeanFeldman how would you call the IManageSubscriptions new BulkSubscribe method without an matching API on the message session? The mention of send was not related to the bulk subscription topic but to bulk operations in general.

SeanFeldman commented 7 years ago

Due to the nature of those events (auto-subscribed upon endpoint startup), possible candidates:

andreasohlund commented 7 years ago

I was talking about the endgame, sorry should have been more clear. In a world where transports fully manage the pubsub part I don't see how the transport not being started would be an issue.

Not against considering the bulk subscribe idea in the interim On Fri, 21 Apr 2017 at 18:36, Tim Bussmann notifications@github.com wrote:

so the core currently only subscribes via the AutoSubscribe startup task which is done when the endpoint is essentially started but before it starts consuming messages. Passing subscribers earlier would probably be quite a change and only works with native pub/sub transports and not with message driven pub/sub (as it requires a started transport by it's nature) so that sounds like a much larger change compared to my proposal?

@SeanFeldman https://github.com/SeanFeldman how would you call the IManageSubscriptions new BulkSubscribe method without an matching API on the message session? The mention of send was not related to the bulk subscription topic but to bulk operations in general.

— You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub https://github.com/Particular/NServiceBus/issues/4632#issuecomment-296240556, or mute the thread https://github.com/notifications/unsubscribe-auth/AAHoZIVt5V2ZcILytH_K5wKgYdT36VM2ks5ryNsOgaJpZM4NDOZz .

timbussmann commented 7 years ago

@andreasohlund yeah, long term I agree with you, but that seems something for v8?

@SeanFeldman sorry for being unclear, I meant how would you call this method from code without a matching API in the message session? :)

SeanFeldman commented 7 years ago

@timbussmann there's some sort of confusion. What I'm referring to is not related to the message session, but is more aligned with a long term (what @andreasohlund is talking about). This would not be a user triggered operation.

bording commented 7 years ago

@SeanFeldman Take a look at how AutoSubscribe is implemented: https://github.com/Particular/NServiceBus/blob/127b10eb48efcc90eb4d5de7d9645535445d8aeb/src/NServiceBus.Core/Routing/AutomaticSubscriptions/AutoSubscribe.cs#L71

It only has access to an IMessageSession, so it just sends multiple Subscribe messages. The outgoing subscription pipeline is invoked for each subscription. The terminator of that pipeline is either the MessageDrivenSubscribeTerminator or the NativeSubscribeTerminator.

Because of how this works, there isn't currently a way to batch those subscriptions without adding a way to send a batch of them at once through IMessageSession.

timbussmann commented 7 years ago

exactly what @bording said. And if we talk about future major API changes I think there are two different things mentioned here:

SeanFeldman commented 7 years ago

First, thank you @bording for visualizing the discussion. It helps.

Second, for the ASB there's a workaround for now (more management operations on the publisher side of things, but that should suffice), until Core API can be updated in the future.

danielmarbach commented 7 years ago

@timbussmann I'm not yet convinced that a Send with params is a good simple step towards batching. In the UoW proposal I've outlined a good and solid API with examples for various frameworks which would be a step towards batch and easily allows to hook those batched sessions on things like MVC controllers. Batching itself as I've proved with a few spikes is not that complex to achieve even if core only changes. So I'm not sure why simple needs to be an argument to go with the Send and params based overload. Happy to discuss this further but I think we should not go into this direction just yet.

timbussmann commented 7 years ago

discussed this on the @Particular/nservicebus-maintainers sync and decided to add it to the future milestone as it's a valid request but would require a major version as some kind of breaking changes would be involved. It seems there are multiple potential approaches which would include this feature, e.g.