Skip to content
This repository has been archived by the owner on Jun 23, 2023. It is now read-only.

Rework consumer API #49

Merged
merged 7 commits into from
Jul 15, 2019
Merged

Rework consumer API #49

merged 7 commits into from
Jul 15, 2019

Conversation

yaziine
Copy link

@yaziine yaziine commented Jul 2, 2019

From now the main entry point of the Consumer is New.
New takes a pointer to a Config in parameter, it shouldn't be nil, otherwise, an error will be returned.

Thus Serve needs no parameters. The configuration is handled by New and the brokers's addresses are managed by NewConfig.

The package documentation has been updated too.

This fixes #44.

consumer/config.go Show resolved Hide resolved
@yaziine
Copy link
Author

yaziine commented Jul 5, 2019

@rogpeppe 🆙

@yaziine yaziine removed the request for review from asdine July 9, 2019 03:51
Copy link
Contributor

@rogpeppe rogpeppe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM modulo a few suggestions.

A few other cleanups that could be done:

  • the HandlerConfig type isn't used by the exported API and could be unexported
  • the Consumer.Handle is documented that it must be called before Serve, but ISTM that collection.handlers is only accessed within a mutex and thus it should be OK to call Handle at any time, simplifying the API a little.
  • the methods on the collection type are exported, but I think are only used internally, so could maybe be unexported to make it easier to understand the code.
  • it would be nice if Stop documented whether all outstanding HandleMessage calls will have completed when Stop returns (or not...)

Thanks for doing this, and sorry for the delayed review!

consumer/consumer.go Outdated Show resolved Hide resolved
consumer/consumer.go Show resolved Hide resolved
consumer/config.go Outdated Show resolved Hide resolved
consumer/config.go Outdated Show resolved Hide resolved
consumer/config.go Show resolved Hide resolved
@yaziine yaziine merged commit a3c73dc into master Jul 15, 2019
@yaziine yaziine deleted the rework-consumer-api branch July 15, 2019 06:52
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rework Consumer API
3 participants