[jira] [Commented] (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=16237209#comment-16237209 ] 

Jeffrey Olchovy commented on KAFKA-3049:
----------------------------------------

Saw this was updated. I believe the existing PR can still merge cleanly, but let me know if you want an updated patch / PR. 

> 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.10.0.0, 0.11.0.0, 1.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.4.14#64029)