Allow the dispatcher concurrency to be overriden per channel #1669
Conversation
danielmarbach
force-pushed
the
dispatcher-concurrency-override
branch
3 times, most recently
from
df62f86
to
e3ced40
Compare
|
Maybe for v8 the concurrency can be made a consumer level concern since allows more fine-grained control over having a concurrency per consumer instead of sharing the concurrency from the channel with all the consumers. |
* Change consumer dispatch concurrency value to a `ushort` * Create `CreateChannelAsync` extension to use `DefaultConsumerDispatchConcurrency` * Standardize on `ConsumerDispatchConcurrency` name
lukebakken
force-pushed
the
dispatcher-concurrency-override
branch
from
8d02a94
to
d1101b1
Compare
|
@lukebakken what I find a bit misleading is that my proposal on this PR was different to what you have force pushed to my commit. So effectively you have erased my original proposal with a different approach that now from the history perspective looks like mine and my original proposal is lost. In my change the value of the property was used as a default when no channel specific override was there. So effectively the property value was the default but now things are different or maybe I'm misunderstanding the code because it is already late and I'm glancing at it from my phone. |
|
Don't get me wrong. I'm not feeling entitled in any way. You as a maintainer are free to have it the way you think is best. But wouldn't it then be better to push those changes as individual commits and then squash on merge to preserve that context of the change at least on the PR instead of force pushing? |
|
I didn't mean to change the behavior, just remove the nullable parameter. I want the behavior to use a dispatch concurrency of 1, or what is set in the connection factory, unless So, there is this API on the ...and this extension method: If a user calls Otherwise, a value for Is that OK? |
|
Argh, I see where I screwed up I think. |
|
I think it would also be fine to remove the property on the factory but my intent was to keep it there for now and use that value as a default when not overwritten. And then might be one or any other value if explicitly set. But never only the constant 1 But if it always falls back to just the constant 1 I wonder what the value of having that property on the connection factory is in the first place. Which might the right question to ask. |
|
In hindsight I should have done a better job in expressing my thoughts about the initial idea. Apologies for that |
| /// | ||
| /// Defaults to <see cref="IConnectionFactory.ConsumerDispatchConcurrency"/>. | ||
| /// | ||
| /// For concurrency greater than one this removes the guarantee that consumers handle messages in the order they receive them. |
There was a problem hiding this comment.
It's great to see this mentioned, it's has been a repeated question with the Java client (which has a comparable setting) for many years.
Yep, I see what you mean! I'm preparing a follow-up PR. |
As @danielmarbach points out, it's kind of silly to have the dispatch concurrency set in the connection factory as PR #1669 makes this value per-channel. So, we'll remove it from `IConnectionFactory` and use a default parameter value of `1` in `CreateChannelAsync`
|
Thanks again @danielmarbach - #1671 As for the force-pushes and combination of commits, I'll keep your concerns in mind going forward when I modify contributions! |
PR #1669 by @danielmarbach adds the ability to configure consumer dispatch on a per-channel basis. * Test that consumer dispatch concurrency is set on the dispatcher.
PR #1669 by @danielmarbach adds the ability to configure consumer dispatch on a per-channel basis. * Test that consumer dispatch concurrency is set on the dispatcher.
PR #1669 by @danielmarbach adds the ability to configure consumer dispatch on a per-channel basis. * Test that consumer dispatch concurrency is set on the dispatcher.
PR #1669 by @danielmarbach adds the ability to configure consumer dispatch on a per-channel basis. * Test that consumer dispatch concurrency is set on the dispatcher.
PR #1669 by @danielmarbach adds the ability to configure consumer dispatch on a per-channel basis. * Test that consumer dispatch concurrency is set on the dispatcher.
…llowup Follow-up to #1669 - per-channel dispatch concurrency
Proposed Changes
See #1668
Types of Changes
What types of changes does your code introduce to this project?
Put an
xin the boxes that applyChecklist
Put an
xin the boxes that apply. You can also fill these out after creatingthe PR. If you're unsure about any of them, don't hesitate to ask on the
mailing list. We're here to help! This is simply a reminder of what we are
going to look for before merging your code.
CONTRIBUTING.mddocumentFurther Comments
If this is a relatively large or complex change, kick off the discussion by
explaining why you chose the solution you did and what alternatives you
considered, etc.