Skip to content
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

Shuffle messages less in the producer. #513

Closed
wants to merge 1 commit into from

Conversation

eapache
Copy link
Contributor

@eapache eapache commented Aug 13, 2015

Put them in a map right up front in the aggregator, it only requires tracking
one exta piece of metadata (total messages in the map) and it means we don't
have to shuffle them into this form before constructing the request anyways.

One piece of #433.

Not sure if this is worth landing as-is or if it makes more sense to just rewrite the whole thing at once rather than incrementally.

Put them in a map right up front in the aggregator, it only requires tracking
one exta piece of metadata (total messages in the map) and it means we don't
have to shuffle them into this form before constructing the request anyways.

One piece of #433.
@wvanbergen
Copy link
Contributor

This looks innocent enough to me, but it's hard to look at this wad of code and make sure no changes in behaviour were introduced. Not sure what we can do to overcome that though.

@wvanbergen
Copy link
Contributor

Definitely in favor of doing this one step at a time. After landing this, the second batch of changes to produce requests incrementally will be a lot easier to review.

@eapache eapache closed this Sep 25, 2015
@eapache eapache deleted the producer-fewer-shuffles branch September 25, 2015 17:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants