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

Pluralize the :topic initialization option #17

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

fragoulis
Copy link

@fragoulis fragoulis commented Apr 18, 2019

By default, kafka and kafka-go support consumers subscribing to
multiple topics, by passing an array of strings.

Rafka also respects this.

Rafka-rb simply needs to change the wording in order to respect
the plural and in effect show the api user that consuming from
multiple topics is supported.

The commit takes the extra step of supporting an array of strings
as well as a single string, whether it contains a single topic or
multiple comma-separated topics.

Closes #5

@fragoulis fragoulis requested a review from agis April 18, 2019 11:24
@fragoulis fragoulis self-assigned this Apr 18, 2019
@fragoulis fragoulis force-pushed the plural-topics branch 2 times, most recently from dbcd6c9 to eb2a030 Compare April 18, 2019 11:30
@@ -199,11 +199,17 @@ def commit(*msgs)
# @return [Array<Hash, Hash>] rafka opts, redis opts
def parse_opts(opts)
REQUIRED_OPTS.each do |opt|
raise "#{opt.inspect} option not provided" if opts[opt].nil?
raise "#{opt.inspect} option not provided" if opts[opt].nil? || opt[opt].empty?

Choose a reason for hiding this comment

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

Shouldn't this be opts[opt].empty?

Copy link
Author

Choose a reason for hiding this comment

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

Already fixed.

By default, kafka and kafka-go support consumers subscribing to
multiple topics, by passing an array of strings.

Rafka also respects this (see server.go#parseTopicsAndConfig).

Rafka-rb simply needs to change the wording in order to respect
the plural and in effect show the api user that consuming from
multiple topics is supported.

The commit takes the extra step of supporting an array of strings
as well as a single string, whether it contains a single topic or
multiple comma-separated topics.

Closes #5
Copy link
Contributor

@agis agis left a comment

Choose a reason for hiding this comment

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

I'd prefer to keep backward-compatibility like so:

Rafka::Consumer.new(topic: "greetings")
Rafka::Consumer.new(topic: ["greetings", "foos"]

@fragoulis
Copy link
Author

I'd prefer to keep backward-compatibility like so:

Rafka::Consumer.new(topic: "greetings")
Rafka::Consumer.new(topic: ["greetings", "foos"]

Support both topic & topics ? or just topic?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

consumer: Support consuming from multiple topics
3 participants