Skip to content

Conversation

@efcasado
Copy link
Contributor

@efcasado efcasado commented Apr 2, 2018

This pull-request aims at making it easier to get started with KafkaEx.

I've noticed that some of the settings do not provide sane defaults. This pull-request introduces the following changes:

  • The default worker is disabled by default
  • Sets a sane default value for the :consumer_group option

Without these changes, KafkaEx yields an error at boot time.

Erlang/OTP 20 [erts-9.1.3] [source] [64-bit] [smp:8:8] [ds:8:8:10] [async-threads:10] [hipe] [kernel-poll:false] [dtrace]

=INFO REPORT==== 2-Apr-2018::14:54:55 ===
    application: logger
    exited: stopped
    type: temporary
** (Mix) Could not start application kafka_ex: KafkaEx.start(:normal, []) returned an error: :invalid_consumer_group

lib/kafka_ex.ex Outdated
{:ok, pid} = KafkaEx.Supervisor.start_link(Config.server_impl, max_restarts, max_seconds)

if Application.get_env(:kafka_ex, :disable_default_worker) == true do
if KafkaEx.Config.disable_default_worker do

Choose a reason for hiding this comment

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

Nested modules could be aliased at the top of the invoking module.

@joshuawscott
Copy link
Member

joshuawscott commented Apr 2, 2018

Thanks for the PR!

I don't see any changes that relate to the consumer_group option. Possibly that would be better as another PR in any case 😃

Also, I am unsure that disabling the default worker makes it easier to get started. For example, KafkaEx.fetch/3 uses the default worker :kafka_ex unless the worker_name key/value pair is in the options. This change would require creating a worker and passing that worker name along, or would require setting the configuration disable_default_worker: false, which seems to make it more work to get started with KafkaEx.

That's a pretty significant breaking change, since many people probably use the default worker as a way to get up and running quickly. I am not opposed to making a breaking change, but we definitely would need to change some documentation to reflect the requirement to start your own worker.

@efcasado efcasado force-pushed the sane-default-worker-defaults branch from f733f5c to d34e6de Compare April 2, 2018 15:47
@dantswain
Copy link
Collaborator

@efcasado Thanks for wanting to help make KafkaEx easier to use!

I agree with @joshuawscott here. If the default worker is not enabled by default, then it is not really a "default worker". This was originally done with the intention of making it very easy to "try" KafkaEx with little setup.

It may be worth a discussion about getting rid of the default worker completely. It was added very early on and I have always found it a little misleading, honestly. IMO the user should manage their own processes. This would be a pretty big change, though, and would definitely need a) discussion and b) a major version bump.

@efcasado
Copy link
Contributor Author

efcasado commented Apr 2, 2018

Hey @joshuawscott, thanks for the prompt response 😄

The change to the :consumer_group option can be found here: d34e6de

Perhaps the description of the PR was not clear enough, my bad.

It looks like I misunderstood what the default worker was all about. I thought it was a dummy worker used mostly for toying around with KafkaEx. I'll change the default value for :disable_default_worker to false (note that prior to this PR the default value is nil).

@efcasado efcasado force-pushed the sane-default-worker-defaults branch from 066ee59 to ba86460 Compare April 2, 2018 15:56
@efcasado
Copy link
Contributor Author

efcasado commented Apr 2, 2018

@dantswain Thanks for the clarification. It makes a whole lot of sense. Deprecating the default worker is definitely beyond the scope of this PR. I've just updated the PR to address only the issue with the default values, which was the sole intention of this PR 😄 .

@efcasado efcasado force-pushed the sane-default-worker-defaults branch from 398dfff to ba86460 Compare April 2, 2018 16:05
@joshuawscott
Copy link
Member

👍 This looks good to me, once the merge conflict is fixed

@efcasado efcasado force-pushed the sane-default-worker-defaults branch from ba86460 to 3aaa0bb Compare April 2, 2018 19:02
@efcasado
Copy link
Contributor Author

efcasado commented Apr 2, 2018

I've just rebased it.

@joshuawscott joshuawscott merged commit 662330a into kafkaex:master Apr 2, 2018
@joshuawscott
Copy link
Member

Thanks again @efcasado !

robotarmy pushed a commit to RAM9/kafka_ex that referenced this pull request Apr 7, 2025
…ults

Set sane default values for using the default worker
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.

3 participants