[
https://issues.apache.org/jira/browse/KAFKA-3049?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15074312#comment-15074312
]
ASF GitHub Bot commented on KAFKA-3049:
---------------------------------------
GitHub user jeffreyolchovy opened a pull request:
https://github.com/apache/kafka/pull/719
KAFKA-3049: VerifiableProperties does not respect 'default' properties of
underlying java.util.Properties instance
See https://issues.apache.org/jira/browse/KAFKA-3049 for more information.
An alternative solution can be found at:
https://github.com/apache/kafka/compare/trunk...jeffreyolchovy:KAFKA-3049_immutable-property-names.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/jeffreyolchovy/kafka KAFKA-3049_null-check
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/kafka/pull/719.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #719
----
commit 70360a1030d078fc8d7cd14fd2da129c3b425151
Author: Jeffrey Olchovy <je...@tapad.com>
Date: 2015-12-29T21:06:50Z
add test scaffolding and unit tests for VerifiableProperties
commit 6ded4d8720756977b31e7e20b0d1c3c2ae020997
Author: Jeffrey Olchovy <je...@tapad.com>
Date: 2015-12-29T21:08:55Z
replace containsKey with a null check on getProperty to support default
properties
----
> VerifiableProperties does not respect 'default' properties of underlying
> java.util.Properties instance
> ------------------------------------------------------------------------------------------------------
>
> Key: KAFKA-3049
> URL: https://issues.apache.org/jira/browse/KAFKA-3049
> Project: Kafka
> Issue Type: Bug
> Components: config, core
> Affects Versions: 0.7, 0.8.1.1, 0.8.2.1, 0.9.0.0
> Reporter: Jeffrey Olchovy
> Priority: Minor
> Labels: easyfix
>
> When retrieving values from the underlying {{Properties}} instance with the
> {{getString}}, {{get<Type>}}, etc. methods of a {{VerifiableProperties}}
> instance, a call to the underlying {{Properties.containsKey}} method is made.
> This method will not search the default properties values of the instance,
> rendering any default properties defined on the {{Properties}} instance
> useless.
> A practical example is shown below:
> {noformat}
> // suppose we have a base, default set of properties to supply to all
> consumer groups
> val baseProps = new Properties
> baseProps.setProperty("zookeeper.connect", "localhost:2181/kafka")
> baseProps.setProperty("zookeeper.connection.timeout.ms", "2000")
> // additional we have discrete properties instances for each consumer group
> that utilize these defaults
> val groupProps1 = new Properties(baseProps)
> groupProps1.setProperty("group.id", "test-1")
> val groupProps2 = new Properties(baseProps)
> groupProps2.setProperty("group.id", "test-2")
> // when attempting to create an instance of a high-level Consumer with the
> above properties an exception will be thrown due to the aforementioned
> problem description
> java.lang.IllegalArgumentException: requirement failed: Missing required
> property 'zookeeper.connect'
> at scala.Predef$.require(Predef.scala:233)
> at
> kafka.utils.VerifiableProperties.getString(VerifiableProperties.scala:177)
> at kafka.utils.ZKConfig.<init>(ZkUtils.scala:879)
> at kafka.consumer.ConsumerConfig.<init>(ConsumerConfig.scala:100)
> at kafka.consumer.ConsumerConfig.<init>(ConsumerConfig.scala:104)
> // however, the groupProps instances will return the correct value for
> "zookeeper.connect" when using `Properties.getProperty`
> assert(groupProps1.getProperty("zookeeper.connect", "localhost:2181/kafka"))
> assert(groupProps2.getProperty("zookeeper.connect", "localhost:2181/kafka"))
> {noformat}
> I believe it is worthwhile for Kafka to respect the default properties
> feature of {{java.util.Properties}}, and further, that Kafka should
> discourage the use of the methods on {{Properties}} that are inherited from
> {{Hashtable}} (e.g. {{containsKey}}). One can argue that
> {{VerifiableProperties}} is not 'correct' due to this behavior, but a user
> can always workaround this by creating discrete instances of {{Properties}}
> with a set of default properties manually added to each instance. However,
> this is inconvenient and may only encourage the use of the discouraged
> {{Hashtable}} methods like {{putAll}}.
> Two proposed solutions follow:
> 1. Do not delegate to the {{Properties.containsKey}} method during the
> invocation of {{VerifiableProperties.containsKey}}. One can use a null check
> in conjunction with {{getProperty}} in its place.
> 2. Treat the underlying {{Properties}} instance as immutable and assign the
> result of {{Properties.stringPropertyNames()}} to a member of
> {{VerifiableProperties}}. One can check this set of known, available property
> names, which respects the optional default properties, when
> {{VerifiableProperties.containsKey}} is invoked.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
