-
Notifications
You must be signed in to change notification settings - Fork 593
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
add size hint variable for PublishBatch creation #888
Conversation
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 makes a lot of sense since batches are rarely random in size. However, this PR changes the public API and thus needs an API approval test update
dotnet test -f netcoreapp3.1 projects/Unit --filter "FullyQualifiedName~RabbitMQ.Client.Unit.APIApproval"
I updated |
Backported to |
@michaelklishin This is a breaking change, so it should not have been backported to the When looking at the APIApproval changes, any time you see an interface being modified, that is a breaking change. These are definitely the kinds of changes I'd like to see in new minor versions, so I think that's pointing to us needing to remove interfaces from the public API surface because they prevent these kind of things from being possible. |
Is there also a good reason to have interfaces for the basics like IConnection, IModel etc.? Don't really see a reason why anyone would go through the trouble of implementing their own version of those. |
When I see interfaces used like that, it's usually an attempt to make testing easier since you can provide fake testing implementations. |
I'm not going to lie it is seriously annoying to me that we cannot ship new small features like this quickly in this client because of the adopted semantic versioning interpretation. There is so much code out there that uses interfaces that removing them seems unrealistic. |
I suggest then that we only release new majors and patch releases, no minors. Minors make no sense in the world of strict semantic versioning: even small features often require some kind of interface changes, even minor usability improvements can be interpreted as breaking behavior changes. If almost no change is appropriate for a minor release, why bother with them at all? |
For context, we have a SharedPublishConnection and a SharedConsumerConnection implementation of IConnection that we use to enforce that publish and subscribe happen on separate connections/sockets (to avoid TCP backpressure on consumers in case of backlogs) as well as to enforce a single publisher and single consumer connection per endpoint/connection config (to avoid unnecessary TCP connections). |
Proposed Changes
add a new method with a size hint for the creation of the list to hold the commands. This can reduce the allocations due to the storage of the commands by half.
Types of Changes
Checklist
CONTRIBUTING.md
documentFurther Comments
This only adds a new overloaded method to the public IModel interface.