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

Forbid empty ClientID and use 'sarama' as default #664

Merged
merged 2 commits into from
Jun 1, 2016

Conversation

slaunay
Copy link
Contributor

@slaunay slaunay commented Jun 1, 2016

Documentation states that "sarama" is the default ClientID but the empty string actually is used.

This is a not a problem for connecting to the brokers but it creates issues when settings policies against the tools binaries (e.g. kafka-console-producer) like:

bin/kafka-configs.sh  --zookeeper localhost:2181 --describe --entity-name ""  --entity-type clients
Error while executing topic command Path must not end with / character
java.lang.IllegalArgumentException: Path must not end with / character
    at org.apache.zookeeper.common.PathUtils.validatePath(PathUtils.java:58)
    at org.apache.zookeeper.ZooKeeper.getData(ZooKeeper.java:1137)
    at org.apache.zookeeper.ZooKeeper.getData(ZooKeeper.java:1184)
    at org.I0Itec.zkclient.ZkConnection.readData(ZkConnection.java:119)
    at org.I0Itec.zkclient.ZkClient$12.call(ZkClient.java:1094)
    at org.I0Itec.zkclient.ZkClient$12.call(ZkClient.java:1090)
    at org.I0Itec.zkclient.ZkClient.retryUntilConnected(ZkClient.java:985)
    at org.I0Itec.zkclient.ZkClient.readData(ZkClient.java:1090)
    at org.I0Itec.zkclient.ZkClient.readData(ZkClient.java:1085)
    at org.I0Itec.zkclient.ZkClient.readData(ZkClient.java:1074)
    at kafka.admin.AdminUtils$.fetchEntityConfig(AdminUtils.scala:353)
    at kafka.admin.ConfigCommand$$anonfun$describeConfig$1.apply(ConfigCommand.scala:108)
    at kafka.admin.ConfigCommand$$anonfun$describeConfig$1.apply(ConfigCommand.scala:107)
    at scala.collection.immutable.List.foreach(List.scala:318)
    at kafka.admin.ConfigCommand$.describeConfig(ConfigCommand.scala:107)
    at kafka.admin.ConfigCommand$.main(ConfigCommand.scala:57)
    at kafka.admin.ConfigCommand.main(ConfigCommand.scala)

Fix uses "sarama" as the default ClientID following the documentation and also forbids the empty string (test case provided).

@@ -67,6 +67,8 @@ type Client interface {
}

const (
// Default ClientID
DefaultClientID = "sarama"
Copy link
Contributor

Choose a reason for hiding this comment

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

There's no reason to make this a public constant.

Also, while I understand the intent behind putting it in client.go I think it makes more sense in config.go.

@eapache
Copy link
Contributor

eapache commented Jun 1, 2016

Wow I'm dumb. I had two distinct chances to get this trivial thing right: when I wrote the original PR that said "it defaults to sarama!" and a whole year later when I wrote the regex and had a chance to use the correct quantifier, but didn't.

Thanks for finding this :)

@slaunay
Copy link
Contributor Author

slaunay commented Jun 1, 2016

I applied the changes.
Let me know if you want me to rebase the changes.

Thanks for this sweet library, much easier to read than the official Java client.

@eapache eapache merged commit ee044df into IBM:master Jun 1, 2016
@eapache
Copy link
Contributor

eapache commented Jun 1, 2016

Thanks :)

@slaunay slaunay deleted the bugfix/empty-client-id branch June 1, 2016 17:53
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.

2 participants