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

KAFKA-3945: Change the type of 'acks' config in console producer to String #1618

Closed
wants to merge 1 commit into from

Conversation

vahidhashemian
Copy link
Contributor

@vahidhashemian vahidhashemian commented Jul 12, 2016

The acks config that is provided to console producer with request-required-acks comes with all, -1, 0, 1 as valid options (all and -1 being interchangeable). Currently, the console producer expects an integer for this input and that makes all to become an invalid input. This PR fixes this issue by changing the input type to String.

@omkreddy
Copy link
Contributor

LGTM

1 similar comment
@granthenke
Copy link
Member

LGTM

.ofType(classOf[java.lang.Integer])
.defaultsTo(0)
.ofType(classOf[java.lang.String])
.defaultsTo("0")
Copy link
Contributor

@ijuma ijuma Jul 19, 2016

Choose a reason for hiding this comment

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

We no longer need to call toString in config.requestRequiredAcks.toString (there are two instances of this).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's right. Thanks for catching those. Will update shortly.

@ijuma
Copy link
Contributor

ijuma commented Jul 19, 2016

The PR description seems a bit misleading. We talk of validation of valid input, but it seems to me that we don't do any validation apart from setting the type of the parameter (previously Integer and now String). Is that right? Have we checked that the user gets an appropriate and friendly error if they pass an invalid parameter with the new code?

@vahidhashemian
Copy link
Contributor Author

vahidhashemian commented Jul 19, 2016

That's right, what I meant by validation was verifying that the input type is a string and not an integer. I'll try to make this more clear in the description.

With the new code, if I provide any input other than those accepted I get this error:

org.apache.kafka.common.config.ConfigException: Invalid value ... for configuration acks: String must be one of: all, -1, 0, 1
    at org.apache.kafka.common.config.ConfigDef$ValidString.ensureValid(ConfigDef.java:827)
    at org.apache.kafka.common.config.ConfigDef.parse(ConfigDef.java:427)
    at org.apache.kafka.common.config.AbstractConfig.<init>(AbstractConfig.java:56)
    at org.apache.kafka.common.config.AbstractConfig.<init>(AbstractConfig.java:63)
    at org.apache.kafka.clients.producer.ProducerConfig.<init>(ProducerConfig.java:337)
    at org.apache.kafka.clients.producer.KafkaProducer.<init>(KafkaProducer.java:188)
    at kafka.producer.NewShinyProducer.<init>(BaseProducer.scala:40)
    at kafka.tools.ConsoleProducer$.main(ConsoleProducer.scala:45)
    at kafka.tools.ConsoleProducer.main(ConsoleProducer.scala)

…tring

The `acks` config that is provided to console producer with `request-required-acks` comes with `all`, `-1`, `0`, `1` as valid options (with `all` and `-1` being interchangeable).
Currently, the console producer expects an integer for this input and that makes `all` to become an invalid input. This PR fixes this issue by changing the input type to String.
@vahidhashemian vahidhashemian changed the title KAFKA-3945: Fix validation of 'acks' config in console producer KAFKA-3945: Change the type of 'acks' config in console producer to String Jul 19, 2016
@vahidhashemian
Copy link
Contributor Author

@ijuma The PR was updated. I hope it addresses your concerns. Thanks.

@vahidhashemian
Copy link
Contributor Author

@ijuma Friendly ping :)

@ijuma
Copy link
Contributor

ijuma commented Aug 3, 2016

It would probably be a bit nicer if we didn't print the stacktrace for such cases, but that applies to other parameters too, so perhaps something for another PR. LGTM.

@asfgit asfgit closed this in 227c54f Aug 3, 2016
@ijuma
Copy link
Contributor

ijuma commented Aug 3, 2016

Merged to trunk.

efeg pushed a commit to efeg/kafka that referenced this pull request May 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants