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.

Sign up for GitHub

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

Jump to bottom

Split producer goroutines into sub-structs #300

Closed
eapache opened this issue Feb 24, 2015 · 2 comments
Closed

Split producer goroutines into sub-structs #300

eapache opened this issue Feb 24, 2015 · 2 comments
Labels
maintenance producer

Comments

@eapache
Copy link
Contributor

eapache commented Feb 24, 2015

When I wrote the producer, I kept each of the five pipeline goroutines as methods on the main Producer object for simplicity's sake, however several of them (in particular the flusher and leaderDispatcher) have now grown too large. The problem with this choice is that it makes it difficult to split logical chunks of that code out into helper functions, because the helpers cannot modify state in the parent function without some complicated parameter/return value passing.

My current thoughts around the fix for #294 are going to make this problem even worse, so before we tackle that we should do a small refactor (internal only, no API changes) of the producer to look more like the current consumer layout: small substructs which contain state variables, thus allowing the large goroutines to be broken up into helper functions and make the code flow much more obvious. Nobody likes reading 150-line functions.

Current thoughts include:

  • rename brokerWorker and use it Create struct to manage the messageAggregator and flusher goroutines
  • create a struct to manage the leaderDispatcher goroutines
  • split some helpers out of flusher and leaderDispatcher to make their main loops more understandable

@drdee @yagnik this is a good project for learning more go and getting your hands dirty with the producer design, with very low risk since there should be no functional changes, just code reorg.
This was referenced Feb 24, 2015
Closed
Merged
Closed
@eapache eapache mentioned this issue Apr 25, 2015
Closed
@eapache
Copy link
Contributor Author

eapache commented Apr 25, 2015

At most recent count, this refactor would enable (or at least substantially simplify) #294 and #433.

@es es self-assigned this May 14, 2015
eapache added a commit that referenced this issue Jul 7, 2015
@eapache
Refactor producer goroutines somewhat
252b5c5 
- rename topicDispatcher to dispatcher
- make a topicProducer struct to handle the old partitionDispatcher
- make a partitionProducer struct to handle the old leaderDispatcher
- move and rename methods for a much more readable structure

Part 1 of #300.
@eapache eapache mentioned this issue Jul 7, 2015
Merged
eapache added a commit that referenced this issue Jul 7, 2015
@eapache
Refactor producer goroutines somewhat
071d28e 
- rename topicDispatcher to dispatcher
- make a topicProducer struct to handle the old partitionDispatcher
- make a partitionProducer struct to handle the old leaderDispatcher
- move and rename methods for a much more readable structure

Part 1 of #300.
@eapache eapache unassigned es Jul 10, 2015
eapache added a commit that referenced this issue Aug 13, 2015
@eapache
Structure the aggregator and flusher goroutines
9a9cdfe 
Another chunk of #300. Once this lands the final step will be to break out the
existing massive `run()` methods into useful helper functions now that
parameter-passing isn't such a pain.

In theory no functional changes; just slightly nicer code and stricter typing
around channel direction.
@eapache eapache mentioned this issue Aug 13, 2015
Merged
@eapache
Copy link
Contributor Author

eapache commented Aug 13, 2015

Between #510 #511 and #512 I am considering this done.

@eapache eapache closed this as completed Aug 13, 2015
eapache added a commit that referenced this issue Aug 13, 2015
@eapache
Add #300 to changelog for 1.5
ad7f1f7
aaronkavlie-wf pushed a commit to aaronkavlie-wf/sarama that referenced this issue Aug 18, 2015
@eapache @aaronkavlie-wf
Structure the aggregator and flusher goroutines
4d05c66 
Another chunk of IBM#300. Once this lands the final step will be to break out the
existing massive `run()` methods into useful helper functions now that
parameter-passing isn't such a pain.

In theory no functional changes; just slightly nicer code and stricter typing
around channel direction.
aaronkavlie-wf pushed a commit to aaronkavlie-wf/sarama that referenced this issue Aug 18, 2015
@eapache @aaronkavlie-wf
Add IBM#300 to changelog for 1.5
61b46b3
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
maintenance producer
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@eapache @es