----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25944/#review54609 -----------------------------------------------------------
core/src/main/scala/kafka/consumer/ZookeeperConsumerConnector.scala <https://reviews.apache.org/r/25944/#comment94796> This comment line is for code line 320, better move it above it. core/src/main/scala/kafka/consumer/ZookeeperConsumerConnector.scala <https://reviews.apache.org/r/25944/#comment94797> This comment line is for code line 320, better move it above it. core/src/main/scala/kafka/consumer/ZookeeperConsumerConnector.scala <https://reviews.apache.org/r/25944/#comment94799> Is there a difference between these two: Thread.sleep() TimeUnit.MILLISECONDS.sleep() since we do the former everywhere in the code base. core/src/main/scala/kafka/consumer/ZookeeperConsumerConnector.scala <https://reviews.apache.org/r/25944/#comment94800> Do we still need this variable? core/src/main/scala/kafka/consumer/ZookeeperConsumerConnector.scala <https://reviews.apache.org/r/25944/#comment94802> Can we omit collection.Seq() here can just use groupId = config.groupId? core/src/main/scala/kafka/consumer/ZookeeperConsumerConnector.scala <https://reviews.apache.org/r/25944/#comment94804> merge in a single line core/src/main/scala/kafka/consumer/ZookeeperConsumerConnector.scala <https://reviews.apache.org/r/25944/#comment94806> revert core/src/main/scala/kafka/consumer/ZookeeperConsumerConnector.scala <https://reviews.apache.org/r/25944/#comment94807> revert core/src/main/scala/kafka/tools/ExportOffsets.scala <https://reviews.apache.org/r/25944/#comment94812> Can we actually make the same format for ZK / offsetmanager, making two different formats would make it harder to be parsed since the user needs to know whether ZK or offsetmanager is used. core/src/main/scala/kafka/tools/ExportOffsets.scala <https://reviews.apache.org/r/25944/#comment94810> We can make a parseBrokerList in Utils and use it there, since I have seens this similar parsing logic at multiple places. core/src/main/scala/kafka/tools/ExportOffsets.scala <https://reviews.apache.org/r/25944/#comment94811> You can take a look at KAFKA-686's latest patch which did some cleanup on the util functions; these function may probably merged into Utils. core/src/main/scala/kafka/tools/ImportOffsets.scala <https://reviews.apache.org/r/25944/#comment94813> Ditto as above, can we unify the input offset format? core/src/main/scala/kafka/tools/ImportOffsets.scala <https://reviews.apache.org/r/25944/#comment94814> Ditto as above. core/src/main/scala/kafka/tools/ImportOffsets.scala <https://reviews.apache.org/r/25944/#comment94815> Same as Joel suggested: we can just use config's default values. core/src/main/scala/kafka/tools/OffsetClient.scala <https://reviews.apache.org/r/25944/#comment94816> The apache header is missing. core/src/main/scala/kafka/tools/OffsetClient.scala <https://reviews.apache.org/r/25944/#comment94819> This could be "error". core/src/main/scala/kafka/tools/OffsetClient.scala <https://reviews.apache.org/r/25944/#comment94822> This can be "trace", or we can just print the offset manager id in "debug" if it does not contain error code. core/src/main/scala/kafka/tools/OffsetClient.scala <https://reviews.apache.org/r/25944/#comment94820> Could be "error("Error while connecting to %s:%d for fetching consumer metadata")", since thi is not a general exception. core/src/main/scala/kafka/tools/OffsetClient.scala <https://reviews.apache.org/r/25944/#comment94821> When an exception is thrown and caught here, we should skip the rest of the loop. core/src/main/scala/kafka/tools/OffsetClient.scala <https://reviews.apache.org/r/25944/#comment94823> Could be "error("Error while connecting to offset manager %s")" core/src/main/scala/kafka/tools/OffsetClient.scala <https://reviews.apache.org/r/25944/#comment94824> Some logging inconsistency: broker [%s:%d] broker %s:%d %s:%d core/src/main/scala/kafka/tools/OffsetClient.scala <https://reviews.apache.org/r/25944/#comment94825> Why this API needs to return an Option while the previous one can directly return the reponse? core/src/main/scala/kafka/tools/OffsetClient.scala <https://reviews.apache.org/r/25944/#comment94826> We do not need "Possible cause" any more, just print the exception's message if necessary. core/src/main/scala/kafka/tools/OffsetClient.scala <https://reviews.apache.org/r/25944/#comment94827> Are there any other exceptions that can be caught besides IOExceptions? We need to be careful about always catching Throwable and printing stack trace. - Guozhang Wang On Sept. 23, 2014, 5:48 p.m., Mayuresh Gharat wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/25944/ > ----------------------------------------------------------- > > (Updated Sept. 23, 2014, 5:48 p.m.) > > > Review request for kafka. > > > Bugs: KAFKA-1013 > https://issues.apache.org/jira/browse/KAFKA-1013 > > > Repository: kafka > > > Description > ------- > > OffsetCLient Tool API. ImportZkOffsets and ExportZkOffsets replaced by > ImportOffsets and ExportOffsets > > > Modified the comments in the headers > > > Corrected a value > > > Diffs > ----- > > config/consumer.properties 83847de30d10b6e78bb8de28e0bb925d7c0e6ca2 > core/src/main/scala/kafka/consumer/ZookeeperConsumerConnector.scala > fbc680fde21b02f11285a4f4b442987356abd17b > core/src/main/scala/kafka/tools/ConfigConstants.scala PRE-CREATION > core/src/main/scala/kafka/tools/ExportOffsets.scala PRE-CREATION > core/src/main/scala/kafka/tools/ImportOffsets.scala PRE-CREATION > core/src/main/scala/kafka/tools/OffsetClient.scala PRE-CREATION > > Diff: https://reviews.apache.org/r/25944/diff/ > > > Testing > ------- > > > Thanks, > > Mayuresh Gharat > >
