[jira] [Commented] (KAFKA-5123) Refactor ZkUtils readData* methods

["Balint Molnar (JIRA)" <ji...@apache.org>]

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

Balint Molnar commented on KAFKA-5123:
--------------------------------------

How to refactor zkUtils.readData method:
* move all zkException inside ZkUtils
** Class ConsumerGroupCommand:
*** https://github.com/apache/kafka/blob/trunk/core/src/main/scala/kafka/admin/ConsumerGroupCommand.scala#L342
*** It is easy because we can match for None and then call the printerror(…)
** Class ConsumerOffsetChecker:
*** https://github.com/apache/kafka/blob/trunk/core/src/main/scala/kafka/tools/ConsumerOffsetChecker.scala#L172
*** Maybe we can throw a different exception here/ introduce new one which is not related to zk.
** Class ZkUtils:
*** https://github.com/apache/kafka/blob/trunk/core/src/main/scala/kafka/utils/ZkUtils.scala#L496
*** We can handle this with None
* this method should only return the data without the stat and introduce a new method with name readDataAndStat which returns the data and the stat too, with this one we don’t need to call the annoying ._1 every time. 

How to refactor zkUtils.readDataMaybeNull method:
* Do not return Some(null) convert Some(null) into None with calling Option() instead of Some(). Also rename this method to readData. I do not see why we need a separate method for these two things.
** This method is a little bit tricky because of the Some() no one traits the Some(null)
** For example:
*** Class ConsumerGroupCommand:
**** https://github.com/apache/kafka/blob/trunk/core/src/main/scala/kafka/admin/ConsumerGroupCommand.scala#L272
**** we are calling a substring on null string
*** Class ZookeeperConsumerConnector:
**** https://github.com/apache/kafka/blob/trunk/core/src/main/scala/kafka/consumer/ZookeeperConsumerConnector.scala#L422
**** we are calling toLong on null string
* This method should only return the data without the stat and introduce a new method with name readDataAndStat. 

How to refactor zkUtils.readDataAndVersionMaybeNull method:
* I think we can remove the MaybeNull from it’s name, otherwise this method looks ok to me.

[~ijuma] what do you think? If you agree, I will start to implement this.

> Refactor ZkUtils readData* methods 
> -----------------------------------
>
>                 Key: KAFKA-5123
>                 URL: https://issues.apache.org/jira/browse/KAFKA-5123
>             Project: Kafka
>          Issue Type: Bug
>            Reporter: Balint Molnar
>            Assignee: Balint Molnar
>            Priority: Minor
>
> Usually only the data value is required but every readData method in the ZkUtils returns a Tuple with the data and the stat.
> https://github.com/apache/kafka/pull/2888



--
This message was sent by Atlassian JIRA
(v6.3.15#6346)