[jira] [Comment Edited] (KAFKA-3049) VerifiableProperties does not respect 'default' properties of underlying java.util.Properties instance

["Jeffrey Olchovy (JIRA)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/KAFKA-3049?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15074289#comment-15074289 ] 

Jeffrey Olchovy edited comment on KAFKA-3049 at 12/29/15 9:20 PM:
------------------------------------------------------------------

Can't seem to assign the issue to myself, but I have two patches available that reflect each proposed solution above.

- https://github.com/apache/kafka/compare/trunk...jeffreyolchovy:KAFKA-3049_null-check
- https://github.com/apache/kafka/compare/trunk...jeffreyolchovy:KAFKA-3049_immutable-property-names



was (Author: olchovy@gmail.com):
Can't seem to assign the issue to myself, but I have two patches available that reflects each proposed solution above.

> 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)