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

Remove warning for allow.auto.create.topics with producer #3952

Merged

Conversation

milindl
Copy link
Contributor

@milindl milindl commented Aug 24, 2022

Also improves the documentation around this property.

This solves the following problem:

The property "allow.auto.create.topics" is supposed to be a consumer
property, but we are setting it (and it is affecting the behaviour of)
both the consumer and producer. It gives a warning if we change it in
the producer, but works nevertheless.

(the default value for the producer is true)

One of the effects is that if a user is using their producer as an
adminclient, a call to get metadata for a topic might create that
topic, and if the user specifies allow.auto.create.topics, then they
get a warning.

Unfortunately, we even recommend using a producer with the above
setting (see INTRODUCTION.md).

A knock on effect is that both the go and python clients use a producer
internally for their adminclients so the user has to either live with
a call to GetMetadata creating topics, or with the warning.
(ref: confluent-kafka-go/#780)

The java client only allows this property to be set on the consumer,
which makes it more confusing.

@milindl milindl force-pushed the remove_warning_for_enabletopic branch from d969ab3 to c64de6e Compare September 6, 2022 14:09
@milindl
Copy link
Contributor Author

milindl commented Sep 12, 2022

Hi @edenhill :

I'm stuck as to what's the best solution to the problem described above. I discussed a few different approaches with @emasab / @mhowlett and we thought this was the best way. The other approaches both won't change the property itself. All the approaches are:

  • change allow.auto.create.topics scope in librdkafka to include producer, this removes warning. we will need to change the doc also (this PR)
  • change all clients individually to use consumer rather than producer for their adminclients
  • some other approach where clients use producer by default but use consumer if this config is specified

@edenhill
Copy link
Contributor

edenhill commented Oct 7, 2022

allow.auto.create.topics is used by the producer too, so I think the proper fix is to remove _RK_CONSUMER from the configuration property and update the docstring to also explain the producer behaviour (i.e., referenced topics will be auto-created).

@milindl
Copy link
Contributor Author

milindl commented Oct 10, 2022

Thanks, it sounds good and that's what I already have done in this PR (removed _RK_CONSUMER, and added doc for the different behaviors of this for producer and consumer)

@edenhill
Copy link
Contributor

edenhill commented Nov 7, 2022

Please rebase this on latest master.

Also make sure that you do not edit CONFIGURATION.md directly since it is auto-generated by make.

Move the CHANGELOG entry to the 1.9.3 Enhancements section.

@milindl milindl force-pushed the remove_warning_for_enabletopic branch from c64de6e to a7855a2 Compare November 7, 2022 11:34
@edenhill
Copy link
Contributor

edenhill commented Nov 7, 2022

Make sure to run the style-fixer (on linux, not osx): make style-fix (or make style-fix-changed prior to committing your stuff, since that's quicker)

Copy link
Contributor

@edenhill edenhill left a comment

Choose a reason for hiding this comment

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

Lots of whitespace/style changes. Inspect your diffs before committing.

src/rdkafka_conf.c Outdated Show resolved Hide resolved
src/rdkafka_conf.c Outdated Show resolved Hide resolved
Also improves the documentation around this property.

This solves the following problem:

The property "allow.auto.create.topics" is supposed to be a consumer
property, but we are setting it (and it is affecting the behaviour of)
both the consumer and producer. It gives a warning if we change it in
the producer, but works nevertheless.

(the default value for the producer is true)

One of the effects is that if a user is using their producer as an
adminclient, a call to get metadata for a topic might create that
topic, and if the user specifies allow.auto.create.topics, then they
get a warning.

Unfortunately, we even recommend using a producer with the above
setting (see INTRODUCTION.md).

A knock on effect is that both the go and python clients use a producer
internally for their adminclients so the user has to either live with
a call to GetMetadata creating topics, or with the warning.

The java client only allows this property to be set on the consumer,
which makes it more confusing.
@milindl milindl force-pushed the remove_warning_for_enabletopic branch from a7855a2 to 3c78315 Compare November 7, 2022 15:57
@milindl
Copy link
Contributor Author

milindl commented Nov 7, 2022

Fixed whitespace issues. Sorry about that, clang-format v14.0.0 did not work as expected, had to use v10.0.0.

@milindl milindl requested a review from edenhill November 7, 2022 16:21
@edenhill edenhill merged commit 42e530c into confluentinc:master Nov 8, 2022
@edenhill
Copy link
Contributor

edenhill commented Nov 8, 2022

LGTM

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.

None yet

2 participants