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

Producer refactor 3 #551

Merged
merged 1 commit into from
Oct 16, 2015
Merged

Producer refactor 3 #551

merged 1 commit into from
Oct 16, 2015

Conversation

eapache
Copy link
Contributor

@eapache eapache commented Oct 14, 2015

@wvanbergen depends on #550, I'll rebase once that's merged.

This is the final step, it moves the request/response handling and all state into the "aggregator" (which is now named the brokerProducer). This makes what used to be the flusher really trivial, so it gets inlined into the constructor.

Most of the logic stays the same, the most confusing change is probably around forcing a blocking flush, because we now have to handle the case where we receive a response while we are waiting, and that changes our state (see the new waitForSpace method).

@eapache eapache force-pushed the producer-refactor-3 branch 3 times, most recently from 2fc0f3f to 613c2d1 Compare October 15, 2015 02:58
@eapache
Copy link
Contributor Author

eapache commented Oct 15, 2015

Rebased on master now that part 2 is merged. The diff is still pretty yucky, but I'm not sure how to split it any further. It may be easier to just read the brokerProducer code in the new version from scratch, without worrying about the old version.

Last piece of this refactor, I'm not sure I can break it up any more.
@eapache eapache force-pushed the producer-refactor-3 branch from 613c2d1 to b3cb7ca Compare October 15, 2015 03:01
@eapache
Copy link
Contributor Author

eapache commented Oct 15, 2015

Landing this will close #433 and should let me approach #294 without tearing my hair out (finally).

@wvanbergen
Copy link
Contributor

👍

With regards to #433 - this doesn't really construct a ProduceRequest incrementally, because that is still done from scratch in produceSet.buildRequest. Can this be done incrementally as we add messages to the produceset?

@eapache
Copy link
Contributor Author

eapache commented Oct 16, 2015

because that is still done from scratch in produceSet.buildRequest

Not from scratch - the MessageSets are built incrementally. The request itself could be build incrementally for uncompressed requests, but for compressed requests we have to wait until we're ready in order to compress the body all at once, so that would be weird. If snappy lib provided an incremental stream compressor we might be able to do both incrementally, but it's not worth the effort at this point.

eapache added a commit that referenced this pull request Oct 16, 2015
@eapache eapache merged commit c54ed1f into master Oct 16, 2015
@eapache eapache deleted the producer-refactor-3 branch October 16, 2015 14:07
@eapache
Copy link
Contributor Author

eapache commented Oct 16, 2015

Still tearing my hair out over #294 - I am learning to hate this state machine

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