-
Notifications
You must be signed in to change notification settings - Fork 595
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Pull ProcessingConcurrency into connection factory interface #899
Pull ProcessingConcurrency into connection factory interface #899
Conversation
/// </summary> | ||
/// <remarks>For concurrency greater than one this removes the guarantee that consumers handle messages in the order they receive them. | ||
/// In addition to that consumers need to be thread/concurrency safe.</remarks> | ||
int ProcessingConcurrency { get; set; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we call this ConsumerDispatchConcurrency
or similar? "Processing" is not particularly specific.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This name already passed all the reviews, is on master and also brought into 6.x
. What is causing the sudden name change request? Especially because I asked for this feedback as part of #866 (comment)
Btw. I'm not opposed to what you are proposing. Just wondering why
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment uses "consumer concurrency" which is roughly what I'm proposing. Things can easily be changed in master.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't participated in #866 and not every member of our team or community cares equally about naming. So as soon as I've noticed it I tried to suggest something clearer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok. I guess 6.2 isn't out yet so we could still fix it right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about we merge this PR and then do the rename PR later so that we can also backport the rename?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we can still change the name for 6.2
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Renaming can be done in another PR, no problem at all.
Proposed Changes
For v7. pulls now the processing conccurency property up to the connection factory interface
Types of Changes
What types of changes does your code introduce to this project?
Put an
x
in the boxes that applyChecklist
Put an
x
in 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.md
documentFurther Comments
Should not be backported