Nservicebus: New satellite configuration code ignores provided `PushRuntimeSettings`
Who is affected
Symptoms
Analysis
When creating a satellite using the FeatureConfigurationContext.AddSatelliteReceiver API you need to provide an instance of PushRuntimeSettings which allows you to set the max concurrency for the satellite receiver.
This value is never used. When the satellite is started the PushRuntimeSettings from the default receiver is used.
mikeminutillo
All 26 comments
@Particular/nservicebus-maintainers this seems like a 6.1 worthy bug to fix? (or would we even say 6.0.1)
andreasohlund
on 12 Oct 2016
agree on a minor. Doesn't seem to match the critical bug criteria?
timbussmann
on 12 Oct 2016
fixing this also means that core satellites (timeout manager) ignores configured concurrency settings and always will use the default concurrency as of now. Since this code is under own control, I'd say that's no issue but not sure whether I'm missing something?
timbussmann
on 14 Oct 2016
That was the plan, to decouple that setting from the user one.
If the the default (max(2,virtualcores) is suitable for the TM satellites can be discussed though
On 14 Oct 2016, at 11:09, Tim Bussmann [email protected] wrote:
fixing this also means that core satellites (timeout manager) ignores configured concurrency settings and always will use the default concurrency as of now. Since this code is under own control, I'd say that's no issue but not sure whether I'm missing something?
—
You are receiving this because you are on a team that was mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
andreasohlund
on 14 Oct 2016
4222 fixes this issue as discussed so far in this issue. I want to include transport maintainers to have a look at this proposal and give some feedback whether they would agree with this approach or whether we should discuss the issue again.
With the fix proposed in the PR:
- Satellites may have a higher concurrency than the main pipeline (e.g. when limiting to one message at a time).
As our satellites are built with concurrency in mind, this should be no issue even when the user code cannot handle concurrent messages. The only potential impact would be when not using transactions at all, but I can't think of a scenario where this would change the behavior in a notable way.
- Satellites may have a lower concurrency than the main pipeline
This could be an issue with endpoints processing a lot of messages and using many deferred messages. I would argue that we can assume that our pipelines are quite a bit faster than message handler invoking business logic and a difference may not be an issue unless we're really facing extremes, but then it would be an issue
What's your thought on this transport maintainers? @Particular/rabbitmq-transport-maintainers @Particular/sqlserver-transport-maintainers @Particular/azure-service-bus-maintainers @Particular/azure-storage-queues-maintainers
timbussmann
on 19 Oct 2016
This could be an issue with endpoints processing a lot of messages and using many deferred messages. I would argue that we can assume that our pipelines are quite a bit faster than message handler invoking business logic and a difference may not be an issue unless we're really facing extremes, but then it would be an issue
I'd say that if this is an issue the "owner" of the satellites (TM, Gateway) etc should provide a custom api to allow users to tweak those underlying settings to their liking.
Eg. .Gateway().MaxParallellRemoteSiteCalls(32) ?
andreasohlund
on 24 Oct 2016
I don't have enough context to comment on this on behalf of @Particular/rabbitmq-transport-maintainers . I'd be happy to jump on a call, or perhaps one of the other members can comment.
adamralph
on 24 Oct 2016
@Particular/rabbitmq-transport-maintainers @Particular/sqlserver-transport-maintainers @Particular/azure-service-bus-maintainers @Particular/azure-storage-queues-maintainers ping
timbussmann
on 25 Oct 2016
If I understand the approach correctly now we can specify concurrency settings per satellite. If that is the case I like the proposal.
tmasternak
on 25 Oct 2016
If I understand the approach correctly now we can specify concurrency settings per satellite. If that is the case I like the proposal
@tmasternak yes, but also the concurrency configured by the user is not passed to the satellites, so they will always run with the default concurrency settings. That may be addressed as mentioned by andreas by providing additional configuration APIs for the core satellites, but that needs to be handled by every user adding satellites.
timbussmann
on 25 Oct 2016
Isn't this change completely transparent to the transport?
MarcinHoppe
on 25 Oct 2016
@timbussmann sounds good. I think that if a user should be able to configure the concurrency for a satellite a dedicated API (as mentioned by @andreasohlund) should be added.
tmasternak
on 25 Oct 2016
Isn't this change completely transparent to the transport?
@MarcinHoppe not necessarily. E.g. for azure the concurrency settings may have direct impact on the costs.
timbussmann
on 25 Oct 2016
@timbussmann OK, got it. Thanks!
MarcinHoppe
on 25 Oct 2016
Making this change along with providing an API to override it an a per-satellite basis should cover all the possible scenarios.
bording
on 25 Oct 2016
Just to clarify:
The feature owning the rekevant satellite would provide an api if needed. Ie we would not have a generic public facing api for this right?
On 25 Oct 2016, at 18:53, Brandon Ording [email protected] wrote:
Making this change along with providing an API to override it an a per-satellite basis should cover all the possible scenarios.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
andreasohlund
on 25 Oct 2016
The feature owning the rekevant satellite would provide an api if needed. Ie we would not have a generic public facing api for this right?
That's my understanding, though I think it makes sense to consider going ahead and adding them now.
bording
on 25 Oct 2016
Why now?
Do we have any reports of users asking for it?
On 25 Oct 2016, at 23:11, Brandon Ording [email protected] wrote:
The feature owning the rekevant satellite would provide an api if needed. Ie we would not have a generic public facing api for this right?
That's my understanding, though I think it makes sense to consider going ahead and adding them now.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
andreasohlund
on 26 Oct 2016
doesn't seem to be a huge effort to add a configuration API?
timbussmann
on 26 Oct 2016
What did you have in mind?
.TimoutManager().Xyz(10) ?
and Gateway().Yyz(5) ?
Let's treat that as a different issue at least since it doesn't seem connected with this bug?
andreasohlund
on 26 Oct 2016
yeah I think that's what we would end up with. First thought was to just use some settings key as a backdoor API but then, adding a proper code first API doesn't seem to be real additional effort.
Let's treat that as a different issue at least since it doesn't seem connected with this bug?
definitely wanted to raise a dedicated PR, but why not connect it with this bug?
timbussmann
on 27 Oct 2016
reopening this as the additional API hasn't been added yet (was automatically closed by the merged PR)
timbussmann
on 31 Oct 2016
Sorry but I still don't see how adding those new apis to the TM and Gateway has anything to do with this bug?
It doesn't seem urgent to add them? (Lets raise new issues in the relevant repos and treat them separately)
andreasohlund
on 31 Oct 2016
Agree with @andreasohlund. We can have dedicated issues representing that
work independent from the bug discussed here
On Monday, October 31, 2016, Andreas Öhlund [email protected]
wrote:
Sorry but I still don't see how adding those new apis to the TM and
Gateway has anything to do with this bug?It doesn't seem urgent to add them? (Lets raise new issues in the relevant
repos and treat them separately)—
You are receiving this because you modified the open/close state.
Reply to this email directly, view it on GitHub
https://github.com/Particular/NServiceBus/issues/4213#issuecomment-257347652,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAKospefxxvV7tdt8OPwmYMNU2RcRPcDks5q5hq0gaJpZM4KUajn
.
danielmarbach
on 31 Oct 2016
I'm good with raising dedicated issues, just wanted to make sure this doesn't fall between the gaps. Will raise the issues and close this again.
timbussmann
on 1 Nov 2016
Issues raised. Closing this again.
timbussmann
on 1 Nov 2016
Related issues
tmasternak
·
6Comments
timbussmann
·
10Comments
andreasohlund
·
7Comments
ramonsmits
·
5Comments
WojcikMike
·
5Comments
Most helpful comment
I'd say that if this is an issue the "owner" of the satellites (TM, Gateway) etc should provide a custom api to allow users to tweak those underlying settings to their liking.
Eg.
.Gateway().MaxParallellRemoteSiteCalls(32)?