You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@kafka.apache.org by Andrii Biletskyi <an...@stealth.ly> on 2014/12/22 09:57:39 UTC

Review Request 29301: Patch for KAFKA-1694

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29301/
-----------------------------------------------------------

Review request for kafka.


Bugs: KAFKA-1694
    https://issues.apache.org/jira/browse/KAFKA-1694


Repository: kafka


Description
-------

KAFKA-1694 - introduced new type for Wire protocol, ported ClusterMetadataResponse to it


KAFKA-1694 - Split Admin RQ/RP to separate messages


Diffs
-----

  bin/kafka.sh PRE-CREATION 
  bin/windows/kafka.bat PRE-CREATION 
  build.gradle 18f86e4c8a10618d50ac78572d119c6e100ed85b 
  clients/src/main/java/org/apache/kafka/common/protocol/ApiKeys.java 109fc965e09b2ed186a073351bd037ac8af20a4c 
  clients/src/main/java/org/apache/kafka/common/protocol/Protocol.java 7517b879866fc5dad5f8d8ad30636da8bbe7784a 
  clients/src/main/java/org/apache/kafka/common/protocol/types/MaybeOf.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/protocol/types/Struct.java 121e880a941fcd3e6392859edba11a94236494cc 
  clients/src/main/java/org/apache/kafka/common/requests/ClusterMetadataRequest.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/requests/ClusterMetadataResponse.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/requests/admin/AlterTopicRequest.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/requests/admin/AlterTopicResponse.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/requests/admin/CreateTopicRequest.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/requests/admin/CreateTopicResponse.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/requests/admin/DeleteTopicRequest.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/requests/admin/DeleteTopicResponse.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/requests/admin/DescribeTopicOutput.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/requests/admin/DescribeTopicRequest.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/requests/admin/DescribeTopicResponse.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/requests/admin/ListTopicsOutput.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/requests/admin/ListTopicsRequest.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/requests/admin/ListTopicsResponse.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/requests/admin/TopicConfigDetails.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/requests/admin/TopicPartitionsDetails.java PRE-CREATION 
  clients/src/test/java/org/apache/kafka/common/requests/RequestResponseTest.java df37fc6d8f0db0b8192a948426af603be3444da4 
  core/src/main/scala/kafka/api/ApiUtils.scala 1f80de1638978901500df808ca5133308c9d1fca 
  core/src/main/scala/kafka/api/ClusterMetadataRequest.scala PRE-CREATION 
  core/src/main/scala/kafka/api/ClusterMetadataResponse.scala PRE-CREATION 
  core/src/main/scala/kafka/api/RequestKeys.scala c24c0345feedc7b9e2e9f40af11bfa1b8d328c43 
  core/src/main/scala/kafka/api/admin/AlterTopicRequest.scala PRE-CREATION 
  core/src/main/scala/kafka/api/admin/AlterTopicResponse.scala PRE-CREATION 
  core/src/main/scala/kafka/api/admin/CreateTopicRequest.scala PRE-CREATION 
  core/src/main/scala/kafka/api/admin/CreateTopicResponse.scala PRE-CREATION 
  core/src/main/scala/kafka/api/admin/DeleteTopicRequest.scala PRE-CREATION 
  core/src/main/scala/kafka/api/admin/DeleteTopicResponse.scala PRE-CREATION 
  core/src/main/scala/kafka/api/admin/DescribeTopicRequest.scala PRE-CREATION 
  core/src/main/scala/kafka/api/admin/DescribeTopicResponse.scala PRE-CREATION 
  core/src/main/scala/kafka/api/admin/ListTopicsRequest.scala PRE-CREATION 
  core/src/main/scala/kafka/api/admin/ListTopicsResponse.scala PRE-CREATION 
  core/src/main/scala/kafka/common/AdminRequestFailedException.scala PRE-CREATION 
  core/src/main/scala/kafka/common/ErrorMapping.scala eedc2f5f21dd8755fba891998456351622e17047 
  core/src/main/scala/kafka/common/InvalidRequestTargetException.scala PRE-CREATION 
  core/src/main/scala/kafka/controller/ControllerChannelManager.scala eb492f00449744bc8d63f55b393e2a1659d38454 
  core/src/main/scala/kafka/controller/KafkaController.scala 66df6d2fbdbdd556da6bea0df84f93e0472c8fbf 
  core/src/main/scala/kafka/server/KafkaApis.scala 2a1c0326b6e6966d8b8254bd6a1cb83ad98a3b80 
  core/src/main/scala/kafka/server/MetadataCache.scala bf81a1ab88c14be8697b441eedbeb28fa0112643 
  core/src/main/scala/kafka/server/TopicCommandHelper.scala PRE-CREATION 
  core/src/test/scala/unit/kafka/api/RequestResponseSerializationTest.scala cd16ced5465d098be7a60498326b2a98c248f343 
  settings.gradle 83f764e6a4a15a5fdba232dce74a369870f26b45 
  tools/src/main/java/org/apache/kafka/cli/BaseCommandOpts.java PRE-CREATION 
  tools/src/main/java/org/apache/kafka/cli/Boot.java PRE-CREATION 
  tools/src/main/java/org/apache/kafka/cli/RequestDispatcher.java PRE-CREATION 
  tools/src/main/java/org/apache/kafka/cli/Shell.java PRE-CREATION 
  tools/src/main/java/org/apache/kafka/cli/command/AlterTopicCommand.java PRE-CREATION 
  tools/src/main/java/org/apache/kafka/cli/command/ClearScreenCommand.java PRE-CREATION 
  tools/src/main/java/org/apache/kafka/cli/command/Command.java PRE-CREATION 
  tools/src/main/java/org/apache/kafka/cli/command/CreateTopicCommand.java PRE-CREATION 
  tools/src/main/java/org/apache/kafka/cli/command/DeleteTopicCommand.java PRE-CREATION 
  tools/src/main/java/org/apache/kafka/cli/command/DescribeTopicCommand.java PRE-CREATION 
  tools/src/main/java/org/apache/kafka/cli/command/ExitCommand.java PRE-CREATION 
  tools/src/main/java/org/apache/kafka/cli/command/ListTopicsCommand.java PRE-CREATION 
  tools/src/main/java/org/apache/kafka/cli/command/PrintHelpCommand.java PRE-CREATION 
  tools/src/main/java/org/apache/kafka/cli/command/TopicSwitchCommand.java PRE-CREATION 
  tools/src/main/java/org/apache/kafka/cli/util/StringUtils.java PRE-CREATION 

Diff: https://reviews.apache.org/r/29301/diff/


Testing
-------


Thanks,

Andrii Biletskyi


Re: Review Request 29301: Patch for KAFKA-1694

Posted by Jeff Holoman <je...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29301/#review66838
-----------------------------------------------------------



tools/src/main/java/org/apache/kafka/cli/command/AlterTopicCommand.java
<https://reviews.apache.org/r/29301/#comment110502>

    minor nit: grammar. "The topic to be created, altered, or described"


- Jeff Holoman


On Dec. 24, 2014, 7:22 p.m., Andrii Biletskyi wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29301/
> -----------------------------------------------------------
> 
> (Updated Dec. 24, 2014, 7:22 p.m.)
> 
> 
> Review request for kafka.
> 
> 
> Bugs: KAFKA-1694
>     https://issues.apache.org/jira/browse/KAFKA-1694
> 
> 
> Repository: kafka
> 
> 
> Description
> -------
> 
> KAFKA-1694 - introduced new type for Wire protocol, ported ClusterMetadataResponse to it
> 
> 
> KAFKA-1694 - Split Admin RQ/RP to separate messages
> 
> 
> KAFKA-1694 - Admin commands can be handled only by controller; DeleteTopicCommand NPE fix
> 
> 
> Diffs
> -----
> 
>   bin/kafka.sh PRE-CREATION 
>   bin/windows/kafka.bat PRE-CREATION 
>   build.gradle 18f86e4c8a10618d50ac78572d119c6e100ed85b 
>   clients/src/main/java/org/apache/kafka/common/protocol/ApiKeys.java 109fc965e09b2ed186a073351bd037ac8af20a4c 
>   clients/src/main/java/org/apache/kafka/common/protocol/Protocol.java 7517b879866fc5dad5f8d8ad30636da8bbe7784a 
>   clients/src/main/java/org/apache/kafka/common/protocol/types/MaybeOf.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/protocol/types/Struct.java 121e880a941fcd3e6392859edba11a94236494cc 
>   clients/src/main/java/org/apache/kafka/common/requests/ClusterMetadataRequest.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/ClusterMetadataResponse.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/AlterTopicRequest.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/AlterTopicResponse.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/CreateTopicRequest.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/CreateTopicResponse.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/DeleteTopicRequest.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/DeleteTopicResponse.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/DescribeTopicOutput.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/DescribeTopicRequest.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/DescribeTopicResponse.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/ListTopicsOutput.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/ListTopicsRequest.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/ListTopicsResponse.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/TopicConfigDetails.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/TopicPartitionsDetails.java PRE-CREATION 
>   clients/src/test/java/org/apache/kafka/common/requests/RequestResponseTest.java df37fc6d8f0db0b8192a948426af603be3444da4 
>   core/src/main/scala/kafka/api/ApiUtils.scala 1f80de1638978901500df808ca5133308c9d1fca 
>   core/src/main/scala/kafka/api/ClusterMetadataRequest.scala PRE-CREATION 
>   core/src/main/scala/kafka/api/ClusterMetadataResponse.scala PRE-CREATION 
>   core/src/main/scala/kafka/api/RequestKeys.scala c24c0345feedc7b9e2e9f40af11bfa1b8d328c43 
>   core/src/main/scala/kafka/api/admin/AlterTopicRequest.scala PRE-CREATION 
>   core/src/main/scala/kafka/api/admin/AlterTopicResponse.scala PRE-CREATION 
>   core/src/main/scala/kafka/api/admin/CreateTopicRequest.scala PRE-CREATION 
>   core/src/main/scala/kafka/api/admin/CreateTopicResponse.scala PRE-CREATION 
>   core/src/main/scala/kafka/api/admin/DeleteTopicRequest.scala PRE-CREATION 
>   core/src/main/scala/kafka/api/admin/DeleteTopicResponse.scala PRE-CREATION 
>   core/src/main/scala/kafka/api/admin/DescribeTopicRequest.scala PRE-CREATION 
>   core/src/main/scala/kafka/api/admin/DescribeTopicResponse.scala PRE-CREATION 
>   core/src/main/scala/kafka/api/admin/ListTopicsRequest.scala PRE-CREATION 
>   core/src/main/scala/kafka/api/admin/ListTopicsResponse.scala PRE-CREATION 
>   core/src/main/scala/kafka/common/AdminRequestFailedException.scala PRE-CREATION 
>   core/src/main/scala/kafka/common/ErrorMapping.scala eedc2f5f21dd8755fba891998456351622e17047 
>   core/src/main/scala/kafka/common/InvalidRequestTargetException.scala PRE-CREATION 
>   core/src/main/scala/kafka/controller/ControllerChannelManager.scala eb492f00449744bc8d63f55b393e2a1659d38454 
>   core/src/main/scala/kafka/controller/KafkaController.scala 66df6d2fbdbdd556da6bea0df84f93e0472c8fbf 
>   core/src/main/scala/kafka/server/KafkaApis.scala 2a1c0326b6e6966d8b8254bd6a1cb83ad98a3b80 
>   core/src/main/scala/kafka/server/MetadataCache.scala bf81a1ab88c14be8697b441eedbeb28fa0112643 
>   core/src/main/scala/kafka/server/TopicCommandHelper.scala PRE-CREATION 
>   core/src/test/scala/unit/kafka/api/RequestResponseSerializationTest.scala cd16ced5465d098be7a60498326b2a98c248f343 
>   settings.gradle 83f764e6a4a15a5fdba232dce74a369870f26b45 
>   tools/src/main/java/org/apache/kafka/cli/BaseCommandOpts.java PRE-CREATION 
>   tools/src/main/java/org/apache/kafka/cli/Boot.java PRE-CREATION 
>   tools/src/main/java/org/apache/kafka/cli/RequestDispatcher.java PRE-CREATION 
>   tools/src/main/java/org/apache/kafka/cli/Shell.java PRE-CREATION 
>   tools/src/main/java/org/apache/kafka/cli/command/AlterTopicCommand.java PRE-CREATION 
>   tools/src/main/java/org/apache/kafka/cli/command/ClearScreenCommand.java PRE-CREATION 
>   tools/src/main/java/org/apache/kafka/cli/command/Command.java PRE-CREATION 
>   tools/src/main/java/org/apache/kafka/cli/command/CreateTopicCommand.java PRE-CREATION 
>   tools/src/main/java/org/apache/kafka/cli/command/DeleteTopicCommand.java PRE-CREATION 
>   tools/src/main/java/org/apache/kafka/cli/command/DescribeTopicCommand.java PRE-CREATION 
>   tools/src/main/java/org/apache/kafka/cli/command/ExitCommand.java PRE-CREATION 
>   tools/src/main/java/org/apache/kafka/cli/command/ListTopicsCommand.java PRE-CREATION 
>   tools/src/main/java/org/apache/kafka/cli/command/PrintHelpCommand.java PRE-CREATION 
>   tools/src/main/java/org/apache/kafka/cli/command/TopicSwitchCommand.java PRE-CREATION 
>   tools/src/main/java/org/apache/kafka/cli/util/StringUtils.java PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/29301/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Andrii Biletskyi
> 
>


Re: Review Request 29301: Patch for KAFKA-1694

Posted by Andrii Biletskyi <an...@stealth.ly>.

> On Feb. 3, 2015, 7:14 p.m., Guozhang Wang wrote:
> > clients/src/main/java/org/apache/kafka/common/protocol/ApiKeys.java, line 44
> > <https://reviews.apache.org/r/29301/diff/7/?file=821315#file821315line44>
> >
> >     How about just augmenting OFFSET_FETCH request to return offsets committed by others within the same group?

We decided to remove ConsumerGroupOffsets tool to a separate ticket / KIP to define a comprehensive list of fields that people would want to see in it.


> On Feb. 3, 2015, 7:14 p.m., Guozhang Wang wrote:
> > clients/src/main/java/org/apache/kafka/common/protocol/Protocol.java, lines 498-542
> > <https://reviews.apache.org/r/29301/diff/7/?file=821316#file821316line498>
> >
> >     It seems we can replace with request with OFFSET_FETCH that can get all consumer offsets within the group. And the log-end-offset can be a separate and useful request by itself: we just need to combine these two requests to get the lag.

See above - will be moved to a separate ticket / KIP.


> On Feb. 3, 2015, 7:14 p.m., Guozhang Wang wrote:
> > core/src/main/scala/kafka/api/ApiUtils.scala, line 45
> > <https://reviews.apache.org/r/29301/diff/7/?file=821349#file821349line45>
> >
> >     "reads a list of values of type T"

Removed this code since we declined MaybeOf idea.


> On Feb. 3, 2015, 7:14 p.m., Guozhang Wang wrote:
> > core/src/main/scala/kafka/api/ApiUtils.scala, line 67
> > <https://reviews.apache.org/r/29301/diff/7/?file=821349#file821349line67>
> >
> >     "reads a single value of type T"

See above - removed this code


> On Feb. 3, 2015, 7:14 p.m., Guozhang Wang wrote:
> > core/src/main/scala/kafka/controller/ControllerChannelManager.scala, lines 301-310
> > <https://reviews.apache.org/r/29301/diff/7/?file=821376#file821376line301>
> >
> >     Do not understand the rationale behind this: could you add some comments? Particularly, why we want to send an empty metadata map to the brokers with forceSendBrokerInfo?
> 
> Andrii Biletskyi wrote:
>     Thanks, this is done because on startup we don't send UpdateMetadaRequest (updateMetadataRequestMap is empty) and thus brokers' cache is not filled with brokers and controller. This leads to ClusterMetadataRequest can't be served correctly. 
>     I'm not sure this is the best way to do it, open for suggestions.
> 
> Guozhang Wang wrote:
>     In this case can we just use addUpdateMetadataRequestForBrokers() before calling sendRequestsToBrokers()?
> 
> Andrii Biletskyi wrote:
>     If I understood correctly - addUpdateMetadataRequestForBrokers() is already called, it's just nothing is added to UpdateMetadata. The steps are the following:
>     1. One broker cluster is started (no topics)
>     2. KafkaController.onControllerFailover() is called
>     3. sendUpdateMetadataRequest()
>     4. addUpdateMetadataRequest(): updateMetadataRequest is created foreach controllerContext.partitionLeadershipInfo.keySet (which is empty)
>     5. sendRequestsToBrokers(): we send UpdateMetadata foreach broker from updateMetadataRequestMap (which is empty) -> broker holding a controller's role doesn't receive UpdateMetadataRequest
>     
>     So essentially the problem is that UpdateMetadaRequest holds data about controller, brokers _and_ partitionState but we send UpdateMetadaRequest only if there is partitionState update to be sent.

This is was fixed in  KAFKA-1867 (liveBroker list not updated on a cluster with no topics) - I removed my workaround after rebase


> On Feb. 3, 2015, 7:14 p.m., Guozhang Wang wrote:
> > core/src/main/scala/kafka/controller/KafkaController.scala, lines 973-976
> > <https://reviews.apache.org/r/29301/diff/7/?file=821377#file821377line973>
> >
> >     Ditto above, would be better if some comments are added.

See above - removed this code.


- Andrii


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29301/#review70790
-----------------------------------------------------------


On March 12, 2015, 11:04 a.m., Andrii Biletskyi wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29301/
> -----------------------------------------------------------
> 
> (Updated March 12, 2015, 11:04 a.m.)
> 
> 
> Review request for kafka.
> 
> 
> Bugs: KAFKA-1694
>     https://issues.apache.org/jira/browse/KAFKA-1694
> 
> 
> Repository: kafka
> 
> 
> Description
> -------
> 
> KAFKA-1694 - introduced new type for Wire protocol, ported ClusterMetadataResponse to it
> 
> 
> KAFKA-1694 - Split Admin RQ/RP to separate messages
> 
> 
> KAFKA-1694 - Admin commands can be handled only by controller; DeleteTopicCommand NPE fix
> 
> 
> KAFKA-1776 - Ported ConsumerGroupOffsetChecker
> 
> 
> KAFKA-1776 - Ported PreferredReplicaElectionTool and ReassignPartitionsTool to CLI
> 
> 
> KAFKA-1694 - ReviewBoard 29301 code review fixes
> 
> 
> KAFKA-1694 - Data for ReassignPartitions and PreferredReplicaLeaderElection is in json string
> 
> 
> KAFKA-1694 - Added logging
> 
> 
> KAFKA-1694 - fixed misprint in schema
> 
> 
> KAFKA-1694 - DescribeTopicCommand supports all flags that TopicCommand does
> 
> 
> KAFKA-1694 - Fixed compile error for new Selector constructor
> 
> 
> KAFKA-1694 - Fixed ConsumerGroupChecker sends DescribeTopicResponse instead of ConsumerGroupOffsetsResponse
> 
> 
> KAFKA-1694 - Introduced AbstractAdminRequest/Response to avoid code duplication for sending admin requests / receiving response.
> 
> 
> KAFKA-1694 - Code review fix: /core shouldn't depend on /tools
> 
> 
> KAFKA-1694 - Code review fix: normalized config field in Create- and AlterTopicRequest
> 
> 
> KAFKA-1694 - Remove server RQ/RP messages, clients' classes are used instead
> 
> 
> KAFKA-1694 - Remove ConsumerGroupOffsets RQ/RP - a separate KIP will be created
> 
> 
> KAFKA-1694 - Remove MaybeOf type, clean up dead code
> 
> 
> KAFKA-1694 - Post rebase merge conflicts fixes
> 
> 
> KAFKA-1694 - Added Protocol errors
> 
> 
> KAFKA-1694 - Bugfix - incorrect error
> 
> 
> Diffs
> -----
> 
>   bin/kafka.sh PRE-CREATION 
>   bin/windows/kafka.bat PRE-CREATION 
>   build.gradle 0f0fe60a74542efa91a0e727146e896edcaa38af 
>   checkstyle/import-control.xml cca4b38ec766028a604f88a1c63228e40df24573 
>   clients/src/main/java/org/apache/kafka/common/ConfigEntry.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/protocol/ApiKeys.java 07aba71303bc1303dbe05e4b121f73f7ad27fdb5 
>   clients/src/main/java/org/apache/kafka/common/protocol/Errors.java ce18a6ce7ed31420cdbec6926a9cd04fa4c806b1 
>   clients/src/main/java/org/apache/kafka/common/protocol/Protocol.java 101f382170ad6740b3f8ff2d27b93a64874a857f 
>   clients/src/main/java/org/apache/kafka/common/requests/ClusterMetadataRequest.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/ClusterMetadataResponse.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/AbstractAdminRequest.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/AbstractAdminResponse.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/AlterTopicRequest.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/AlterTopicResponse.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/CreateTopicRequest.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/CreateTopicResponse.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/DeleteTopicRequest.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/DeleteTopicResponse.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/DescribeTopicOutput.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/DescribeTopicRequest.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/DescribeTopicResponse.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/ListTopicsRequest.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/ListTopicsResponse.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/PreferredReplicaLeaderElectionRequest.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/PreferredReplicaLeaderElectionResponse.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/ReassignPartitionsRequest.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/ReassignPartitionsResponse.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/TopicConfigDetails.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/TopicPartitionDetails.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/VerifyReassignPartitionsRequest.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/VerifyReassignPartitionsResponse.java PRE-CREATION 
>   clients/src/test/java/org/apache/kafka/common/requests/RequestResponseTest.java 13237fd72da5448a3d596b882fef141f336f827d 
>   config/tools-log4j.properties 52f07c96019b4083fc78f62cfb0a81080327e847 
>   core/src/main/scala/kafka/api/ApiUtils.scala 1f80de1638978901500df808ca5133308c9d1fca 
>   core/src/main/scala/kafka/api/ClusterMetadaRequestAndHeader.scala PRE-CREATION 
>   core/src/main/scala/kafka/api/ClusterMetadataResponseAndHeader.scala PRE-CREATION 
>   core/src/main/scala/kafka/api/RequestKeys.scala c24c0345feedc7b9e2e9f40af11bfa1b8d328c43 
>   core/src/main/scala/kafka/api/admin/AlterTopicRequestAndHeader.scala PRE-CREATION 
>   core/src/main/scala/kafka/api/admin/AlterTopicResponseAndHeader.scala PRE-CREATION 
>   core/src/main/scala/kafka/api/admin/CreateTopicRequestAndHeader.scala PRE-CREATION 
>   core/src/main/scala/kafka/api/admin/CreateTopicResponseAndHeader.scala PRE-CREATION 
>   core/src/main/scala/kafka/api/admin/DeleteTopicRequestAndHeader.scala PRE-CREATION 
>   core/src/main/scala/kafka/api/admin/DeleteTopicResponseAndHeader.scala PRE-CREATION 
>   core/src/main/scala/kafka/api/admin/DescribeTopicRequestAndHeader.scala PRE-CREATION 
>   core/src/main/scala/kafka/api/admin/DescribeTopicResponseAndHeader.scala PRE-CREATION 
>   core/src/main/scala/kafka/api/admin/ListTopicsRequestAndHeader.scala PRE-CREATION 
>   core/src/main/scala/kafka/api/admin/ListTopicsResponseAndHeader.scala PRE-CREATION 
>   core/src/main/scala/kafka/api/admin/PreferredReplicaLeaderElectionRequestAndHeader.scala PRE-CREATION 
>   core/src/main/scala/kafka/api/admin/PreferredReplicaLeaderElectionResponseAndHeader.scala PRE-CREATION 
>   core/src/main/scala/kafka/api/admin/ReassignPartitionsRequestAndHeader.scala PRE-CREATION 
>   core/src/main/scala/kafka/api/admin/ReassignPartitionsResponseAndHeader.scala PRE-CREATION 
>   core/src/main/scala/kafka/api/admin/VerifyReassignPartitionsRequestAndHeader.scala PRE-CREATION 
>   core/src/main/scala/kafka/api/admin/VerifyReassignPartitionsResponseAndHeader.scala PRE-CREATION 
>   core/src/main/scala/kafka/common/AdminRequestFailedException.scala PRE-CREATION 
>   core/src/main/scala/kafka/common/ErrorMapping.scala eb1eb4a703098253d0aae79577084569177768d1 
>   core/src/main/scala/kafka/common/NotControllerReceivedAdminRequestException.scala PRE-CREATION 
>   core/src/main/scala/kafka/controller/ControllerChannelManager.scala c582191636f6188c25d62a67ff0315b56f163133 
>   core/src/main/scala/kafka/server/KafkaApis.scala 35af98f0bc1b6a50bd1d97a30147593f8c6a422d 
>   core/src/main/scala/kafka/server/MetadataCache.scala 6aef6e4508ecadbbcc1e12bed2054547b7aa333e 
>   core/src/main/scala/kafka/server/TopicCommandHelper.scala PRE-CREATION 
>   core/src/main/scala/kafka/tools/PreferredReplicaLeaderElectionHelper.scala PRE-CREATION 
>   core/src/main/scala/kafka/tools/ReassignPartitionsHelper.scala PRE-CREATION 
>   core/src/test/scala/unit/kafka/api/RequestResponseSerializationTest.scala fba852afa1b2f46b61e2fd12c38c821ba04e9cc6 
>   settings.gradle 83f764e6a4a15a5fdba232dce74a369870f26b45 
>   tools/src/main/java/org/apache/kafka/cli/BaseCommandOpts.java PRE-CREATION 
>   tools/src/main/java/org/apache/kafka/cli/Boot.java PRE-CREATION 
>   tools/src/main/java/org/apache/kafka/cli/RequestDispatcher.java PRE-CREATION 
>   tools/src/main/java/org/apache/kafka/cli/Shell.java PRE-CREATION 
>   tools/src/main/java/org/apache/kafka/cli/command/AlterTopicCommand.java PRE-CREATION 
>   tools/src/main/java/org/apache/kafka/cli/command/ClearScreenCommand.java PRE-CREATION 
>   tools/src/main/java/org/apache/kafka/cli/command/Command.java PRE-CREATION 
>   tools/src/main/java/org/apache/kafka/cli/command/CreateTopicCommand.java PRE-CREATION 
>   tools/src/main/java/org/apache/kafka/cli/command/DeleteTopicCommand.java PRE-CREATION 
>   tools/src/main/java/org/apache/kafka/cli/command/DescribeTopicCommand.java PRE-CREATION 
>   tools/src/main/java/org/apache/kafka/cli/command/ExitCommand.java PRE-CREATION 
>   tools/src/main/java/org/apache/kafka/cli/command/ListTopicsCommand.java PRE-CREATION 
>   tools/src/main/java/org/apache/kafka/cli/command/PreferredReplicaLeaderElectionCommand.java PRE-CREATION 
>   tools/src/main/java/org/apache/kafka/cli/command/PrintHelpCommand.java PRE-CREATION 
>   tools/src/main/java/org/apache/kafka/cli/command/ReassignPartitionsCommand.java PRE-CREATION 
>   tools/src/main/java/org/apache/kafka/cli/command/TopicSwitchCommand.java PRE-CREATION 
>   tools/src/main/java/org/apache/kafka/cli/util/StringUtils.java PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/29301/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Andrii Biletskyi
> 
>


Re: Review Request 29301: Patch for KAFKA-1694

Posted by Guozhang Wang <wa...@gmail.com>.

> On Feb. 3, 2015, 7:14 p.m., Guozhang Wang wrote:
> > core/src/main/scala/kafka/server/TopicCommandHelper.scala, lines 1-17
> > <https://reviews.apache.org/r/29301/diff/7/?file=821380#file821380line1>
> >
> >     One general comment:
> >     
> >     For some topic commands, why use AdminUtils to write ZK path again instead of handle it via the controller directly? Or this is still WIP?
> 
> Andrii Biletskyi wrote:
>     Not sure I understand you. You mean technially calling ZK client from Controller class, not through TopicCommandHelper? If so - it's just to leave KafkaApi clean and small.
> 
> Guozhang Wang wrote:
>     For example, upon receiving a create-topic request, the helper class will call AdminUtils.createOrUpdateTopicPartitionAssignmentPathInZK() which will just write this request to ZK admin path for it to be captured by controller; however since only the broker with the active controller will receive such requests why don't we just hand off the request from KafkaApi to the controller to handle it.
>     
>     One question, though, is that we need to make sure concurrency is correct for controller handling multiple such tasks, and we have some thoughts about how to deal with such cases (see Jiangjie and my commnets in KAFKA-1305).
> 
> Andrii Biletskyi wrote:
>     Thanks for explanation.
>     So instead of current workflow:
>     CreateTopicRequest -> Helper class -> AdminUtils -> zk path is created -> Controller's changeTopicListener picks up the change -> topic is created
>     You propose:
>     CreateTopicRequest -> Controller directly executes logic from ChangeTopicListener
>     ?
>     Very interesting idea! Can we make a separate ticket for that? I tried to port TopicCommand "as is" in order to have at least for now working end-to-end infrastructure to handle Admin commands. I believe this is more like refactoring TopicCommand (probably delete- and alterTopic should be changed too). I'm a bit concerned adding this refactoring will require additional efforts to test (especially taking into account your note about KAFKA-1305) and time to agree on approach we will use to address this issue.

Agree.


> On Feb. 3, 2015, 7:14 p.m., Guozhang Wang wrote:
> > clients/src/main/java/org/apache/kafka/common/requests/admin/AbstractAdminRequest.java, lines 1-28
> > <https://reviews.apache.org/r/29301/diff/7/?file=821321#file821321line1>
> >
> >     Wondering if an abstract admin request is necessary, as it does not have many common interface functions.
> 
> Andrii Biletskyi wrote:
>     This is needed to avoid code dupliaction in admin clients. See RequestDispatcher for example.
>     You will need to call admin request and get response of that type. Having AbstractAdminRequest (specifically createResponseCounterpart) lets you have:
>     ```
>     public <T extends AbstractAdminResponse> T sendAdminRequest(AbstractAdminRequest<T> abstractRequest) throws Exception {
>     ```
>     Instead of sendCreateTopicRequest, sendAlter... etc. If there is a better and cleaner way to achive this - please let me know.
> 
> Guozhang Wang wrote:
>     I see. How about changing "sendAdminRequest(AbstractAdminRequest<T>)" to "sendRequest(ClientRequest)" and the caller like AlterTopicCommand.execute() will be:
>     
>     AlterTopicRequest alterTopicRequest = // create the request
>     ClientRequest request = new ClientRequest(new RequestSend(...) ...)
>     dispatcher.sendRequest(request)
>     
>     This way we are duplicating the second line here in every upper-level class, while saving the admin interface. I actually do not know which one is better..
> 
> Andrii Biletskyi wrote:
>     Yes, but you will also need typed response. Let me continue your example:
>     
>     AlterTopicRequest alterTopicRequest = // create the request
>     ClientRequest request = new ClientRequest(new RequestSend(...) ...)
>     __ClientResponse response = dispatcher.sendRequest(request, ApiKeys.ALTER_TOPIC)__
>     __AlterTopicResponse alterTopicResponse = new AlterTopicResponse(response.responseBody())__
>     alterTopicResponse.// now get what you need from typed response
>     
>     And you will have this NetworkClient related Stuff (RequestSend, ClientRequest ...) everywhere in you client code. But it looks pretty strange you can't have generic method to send request and get immidiately response of the required type.
>     
>     So really RequestDispatcher allready has sendRequest() as you suggest, with sendAdminRequest I tried to address issue with getting response counterpart. But I agree that solution might mislead people, so if doesn't worth it - I'm okay to remove intermediate AbstractAdminRequest/Response.

Makes sense, I am now OK with the admin request interface.


- Guozhang


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29301/#review70790
-----------------------------------------------------------


On Jan. 14, 2015, 4:07 p.m., Andrii Biletskyi wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29301/
> -----------------------------------------------------------
> 
> (Updated Jan. 14, 2015, 4:07 p.m.)
> 
> 
> Review request for kafka.
> 
> 
> Bugs: KAFKA-1694
>     https://issues.apache.org/jira/browse/KAFKA-1694
> 
> 
> Repository: kafka
> 
> 
> Description
> -------
> 
> KAFKA-1694 - introduced new type for Wire protocol, ported ClusterMetadataResponse to it
> 
> 
> KAFKA-1694 - Split Admin RQ/RP to separate messages
> 
> 
> KAFKA-1694 - Admin commands can be handled only by controller; DeleteTopicCommand NPE fix
> 
> 
> KAFKA-1776 - Ported ConsumerGroupOffsetChecker
> 
> 
> KAFKA-1776 - Ported PreferredReplicaElectionTool and ReassignPartitionsTool to CLI
> 
> 
> KAFKA-1694 - kafka-tools is uploaded on uploadAllArchives
> 
> 
> KAFKA-1694 - ReviewBoard 29301 code review fixes
> 
> 
> KAFKA-1694 - Data for ReassignPartitions and PreferredReplicaLeaderElection is in json string
> 
> 
> KAFKA-1694 - Added logging
> 
> 
> KAFKA-1694 - fixed misprint in schema
> 
> 
> KAFKA-1694 - DescribeTopicCommand supports all flags that TopicCommand does
> 
> 
> KAFKA-1694 - Fixed compile error for new Selector constructor
> 
> 
> KAFKA-1694 - Fixed ConsumerGroupChecker sends DescribeTopicResponse instead of ConsumerGroupOffsetsResponse
> 
> 
> KAFKA-1694 - Introduced AbstractAdminRequest/Response to avoid code duplication for sending admin requests / receiving response.
> 
> 
> Diffs
> -----
> 
>   bin/kafka.sh PRE-CREATION 
>   bin/windows/kafka.bat PRE-CREATION 
>   build.gradle c9ac43378c3bf5443f0f47c8ba76067237ecb348 
>   clients/src/main/java/org/apache/kafka/common/protocol/ApiKeys.java 109fc965e09b2ed186a073351bd037ac8af20a4c 
>   clients/src/main/java/org/apache/kafka/common/protocol/Protocol.java 7517b879866fc5dad5f8d8ad30636da8bbe7784a 
>   clients/src/main/java/org/apache/kafka/common/protocol/types/MaybeOf.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/protocol/types/Struct.java 121e880a941fcd3e6392859edba11a94236494cc 
>   clients/src/main/java/org/apache/kafka/common/requests/ClusterMetadataRequest.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/ClusterMetadataResponse.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/AbstractAdminRequest.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/AbstractAdminResponse.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/AlterTopicRequest.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/AlterTopicResponse.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/ConsumerGroupOffsetsRequest.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/ConsumerGroupOffsetsResponse.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/CreateTopicRequest.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/CreateTopicResponse.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/DeleteTopicRequest.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/DeleteTopicResponse.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/DescribeTopicOutput.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/DescribeTopicRequest.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/DescribeTopicResponse.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/ListTopicsOutput.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/ListTopicsRequest.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/ListTopicsResponse.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/PreferredReplicaLeaderElectionRequest.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/PreferredReplicaLeaderElectionResponse.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/ReassignPartitionsRequest.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/ReassignPartitionsResponse.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/TopicConfigDetails.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/TopicPartitionDetails.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/VerifyPreferredReplicaLeaderElectionRequest.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/VerifyPreferredReplicaLeaderElectionResponse.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/VerifyReassignPartitionsRequest.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/VerifyReassignPartitionsResponse.java PRE-CREATION 
>   clients/src/test/java/org/apache/kafka/common/requests/RequestResponseTest.java df37fc6d8f0db0b8192a948426af603be3444da4 
>   config/tools-log4j.properties 52f07c96019b4083fc78f62cfb0a81080327e847 
>   core/src/main/scala/kafka/api/ApiUtils.scala 1f80de1638978901500df808ca5133308c9d1fca 
>   core/src/main/scala/kafka/api/ClusterMetadataRequest.scala PRE-CREATION 
>   core/src/main/scala/kafka/api/ClusterMetadataResponse.scala PRE-CREATION 
>   core/src/main/scala/kafka/api/RequestKeys.scala c24c0345feedc7b9e2e9f40af11bfa1b8d328c43 
>   core/src/main/scala/kafka/api/admin/AlterTopicRequest.scala PRE-CREATION 
>   core/src/main/scala/kafka/api/admin/AlterTopicResponse.scala PRE-CREATION 
>   core/src/main/scala/kafka/api/admin/ConsumerGroupOffsetsRequest.scala PRE-CREATION 
>   core/src/main/scala/kafka/api/admin/ConsumerGroupOffsetsResponse.scala PRE-CREATION 
>   core/src/main/scala/kafka/api/admin/CreateTopicRequest.scala PRE-CREATION 
>   core/src/main/scala/kafka/api/admin/CreateTopicResponse.scala PRE-CREATION 
>   core/src/main/scala/kafka/api/admin/DeleteTopicRequest.scala PRE-CREATION 
>   core/src/main/scala/kafka/api/admin/DeleteTopicResponse.scala PRE-CREATION 
>   core/src/main/scala/kafka/api/admin/DescribeTopicRequest.scala PRE-CREATION 
>   core/src/main/scala/kafka/api/admin/DescribeTopicResponse.scala PRE-CREATION 
>   core/src/main/scala/kafka/api/admin/ListTopicsRequest.scala PRE-CREATION 
>   core/src/main/scala/kafka/api/admin/ListTopicsResponse.scala PRE-CREATION 
>   core/src/main/scala/kafka/api/admin/PreferredReplicaLeaderElectionRequest.scala PRE-CREATION 
>   core/src/main/scala/kafka/api/admin/PreferredReplicaLeaderElectionResponse.scala PRE-CREATION 
>   core/src/main/scala/kafka/api/admin/ReassignPartitionsRequest.scala PRE-CREATION 
>   core/src/main/scala/kafka/api/admin/ReassignPartitionsResponse.scala PRE-CREATION 
>   core/src/main/scala/kafka/api/admin/VerifyPreferredReplicaLeaderElectionRequest.scala PRE-CREATION 
>   core/src/main/scala/kafka/api/admin/VerifyPreferredReplicaLeaderElectionResponse.scala PRE-CREATION 
>   core/src/main/scala/kafka/api/admin/VerifyReassignPartitionsRequest.scala PRE-CREATION 
>   core/src/main/scala/kafka/api/admin/VerifyReassignPartitionsResponse.scala PRE-CREATION 
>   core/src/main/scala/kafka/common/AdminRequestFailedException.scala PRE-CREATION 
>   core/src/main/scala/kafka/common/ErrorMapping.scala eedc2f5f21dd8755fba891998456351622e17047 
>   core/src/main/scala/kafka/common/InvalidRequestTargetException.scala PRE-CREATION 
>   core/src/main/scala/kafka/controller/ControllerChannelManager.scala eb492f00449744bc8d63f55b393e2a1659d38454 
>   core/src/main/scala/kafka/controller/KafkaController.scala 66df6d2fbdbdd556da6bea0df84f93e0472c8fbf 
>   core/src/main/scala/kafka/server/KafkaApis.scala c011a1b79bd6c4e832fe7d097daacb0d647d1cd4 
>   core/src/main/scala/kafka/server/MetadataCache.scala bf81a1ab88c14be8697b441eedbeb28fa0112643 
>   core/src/main/scala/kafka/server/TopicCommandHelper.scala PRE-CREATION 
>   core/src/main/scala/kafka/tools/ConsumerOffsetCheckerHelper.scala PRE-CREATION 
>   core/src/main/scala/kafka/tools/PreferredReplicaLeaderElectionHelper.scala PRE-CREATION 
>   core/src/main/scala/kafka/tools/ReassignPartitionsHelper.scala PRE-CREATION 
>   core/src/test/scala/unit/kafka/api/RequestResponseSerializationTest.scala cd16ced5465d098be7a60498326b2a98c248f343 
>   settings.gradle 83f764e6a4a15a5fdba232dce74a369870f26b45 
>   tools/src/main/java/org/apache/kafka/cli/BaseCommandOpts.java PRE-CREATION 
>   tools/src/main/java/org/apache/kafka/cli/Boot.java PRE-CREATION 
>   tools/src/main/java/org/apache/kafka/cli/RequestDispatcher.java PRE-CREATION 
>   tools/src/main/java/org/apache/kafka/cli/Shell.java PRE-CREATION 
>   tools/src/main/java/org/apache/kafka/cli/command/AlterTopicCommand.java PRE-CREATION 
>   tools/src/main/java/org/apache/kafka/cli/command/ClearScreenCommand.java PRE-CREATION 
>   tools/src/main/java/org/apache/kafka/cli/command/Command.java PRE-CREATION 
>   tools/src/main/java/org/apache/kafka/cli/command/CreateTopicCommand.java PRE-CREATION 
>   tools/src/main/java/org/apache/kafka/cli/command/DeleteTopicCommand.java PRE-CREATION 
>   tools/src/main/java/org/apache/kafka/cli/command/DescribeTopicCommand.java PRE-CREATION 
>   tools/src/main/java/org/apache/kafka/cli/command/ExitCommand.java PRE-CREATION 
>   tools/src/main/java/org/apache/kafka/cli/command/ListTopicsCommand.java PRE-CREATION 
>   tools/src/main/java/org/apache/kafka/cli/command/PreferredReplicaLeaderElectionCommand.java PRE-CREATION 
>   tools/src/main/java/org/apache/kafka/cli/command/PrintConsumerGroupOffsetsCommand.java PRE-CREATION 
>   tools/src/main/java/org/apache/kafka/cli/command/PrintHelpCommand.java PRE-CREATION 
>   tools/src/main/java/org/apache/kafka/cli/command/ReassignPartitionsCommand.java PRE-CREATION 
>   tools/src/main/java/org/apache/kafka/cli/command/TopicSwitchCommand.java PRE-CREATION 
>   tools/src/main/java/org/apache/kafka/cli/util/StringUtils.java PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/29301/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Andrii Biletskyi
> 
>


Re: Review Request 29301: Patch for KAFKA-1694

Posted by Guozhang Wang <wa...@gmail.com>.

> On Feb. 3, 2015, 7:14 p.m., Guozhang Wang wrote:
> > clients/src/main/java/org/apache/kafka/common/protocol/ApiKeys.java, lines 39-42
> > <https://reviews.apache.org/r/29301/diff/7/?file=821315#file821315line39>
> >
> >     How about merge them into one request? The format could be:
> >     
> >     topic -> string
> >     action -> string (create, alter, delete, describe)
> >     num_partition (?) -> int32
> >     reassignment (?) -> string
> >     configs (?) -> list[string]
> >     
> >     For configs, the client can first get the current config overriden list, and get the new configs list as (curr + added - deleted) so that we do not need two configs field in the request.
> 
> Andrii Biletskyi wrote:
>     I see your point but I'm not sure this will comply Wire protocol use cases. My understanding is that Wire protocol has schema and RQ/RP message is always defined and static.
>     So it's probably possible to merge all requests in one but reusing some of the fields in different types and thus having them different meaning, makes such request type stand out from the others.
>     Also I believe merging all create-alter-delete-describe won't go that smoothly:
>     1) 'describe' has very different _response_ schema than "mutating" 'create-alter-delete'
>     2) I will probably remove MaybeOf type, as guys had concerns, this is different how empty values are handled now (see Jay's comment in KIP discussion) - this will make things even more tangled
>     3) There was a comment about batching commands - I'll probably modify schema so it will accept whitelist for topics (as in TopicCommand), but this is right only for 'alter-delete' - you can't create topic by reqexp - another issue to find the common point
>     4) There is also 'replicas' field which is used only in 'create'
>     
>     As you see messages are really different.

Makes sense, if we are not going the MaybeOf type then there is no point merging them.


> On Feb. 3, 2015, 7:14 p.m., Guozhang Wang wrote:
> > clients/src/main/java/org/apache/kafka/common/requests/admin/AbstractAdminRequest.java, lines 1-28
> > <https://reviews.apache.org/r/29301/diff/7/?file=821321#file821321line1>
> >
> >     Wondering if an abstract admin request is necessary, as it does not have many common interface functions.
> 
> Andrii Biletskyi wrote:
>     This is needed to avoid code dupliaction in admin clients. See RequestDispatcher for example.
>     You will need to call admin request and get response of that type. Having AbstractAdminRequest (specifically createResponseCounterpart) lets you have:
>     ```
>     public <T extends AbstractAdminResponse> T sendAdminRequest(AbstractAdminRequest<T> abstractRequest) throws Exception {
>     ```
>     Instead of sendCreateTopicRequest, sendAlter... etc. If there is a better and cleaner way to achive this - please let me know.

I see. How about changing "sendAdminRequest(AbstractAdminRequest<T>)" to "sendRequest(ClientRequest)" and the caller like AlterTopicCommand.execute() will be:

AlterTopicRequest alterTopicRequest = // create the request
ClientRequest request = new ClientRequest(new RequestSend(...) ...)
dispatcher.sendRequest(request)

This way we are duplicating the second line here in every upper-level class, while saving the admin interface. I actually do not know which one is better..


> On Feb. 3, 2015, 7:14 p.m., Guozhang Wang wrote:
> > core/src/main/scala/kafka/controller/ControllerChannelManager.scala, lines 301-310
> > <https://reviews.apache.org/r/29301/diff/7/?file=821376#file821376line301>
> >
> >     Do not understand the rationale behind this: could you add some comments? Particularly, why we want to send an empty metadata map to the brokers with forceSendBrokerInfo?
> 
> Andrii Biletskyi wrote:
>     Thanks, this is done because on startup we don't send UpdateMetadaRequest (updateMetadataRequestMap is empty) and thus brokers' cache is not filled with brokers and controller. This leads to ClusterMetadataRequest can't be served correctly. 
>     I'm not sure this is the best way to do it, open for suggestions.

In this case can we just use addUpdateMetadataRequestForBrokers() before calling sendRequestsToBrokers()?


> On Feb. 3, 2015, 7:14 p.m., Guozhang Wang wrote:
> > core/src/main/scala/kafka/server/TopicCommandHelper.scala, lines 1-17
> > <https://reviews.apache.org/r/29301/diff/7/?file=821380#file821380line1>
> >
> >     One general comment:
> >     
> >     For some topic commands, why use AdminUtils to write ZK path again instead of handle it via the controller directly? Or this is still WIP?
> 
> Andrii Biletskyi wrote:
>     Not sure I understand you. You mean technially calling ZK client from Controller class, not through TopicCommandHelper? If so - it's just to leave KafkaApi clean and small.

For example, upon receiving a create-topic request, the helper class will call AdminUtils.createOrUpdateTopicPartitionAssignmentPathInZK() which will just write this request to ZK admin path for it to be captured by controller; however since only the broker with the active controller will receive such requests why don't we just hand off the request from KafkaApi to the controller to handle it.

One question, though, is that we need to make sure concurrency is correct for controller handling multiple such tasks, and we have some thoughts about how to deal with such cases (see Jiangjie and my commnets in KAFKA-1305).


- Guozhang


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29301/#review70790
-----------------------------------------------------------


On Jan. 14, 2015, 4:07 p.m., Andrii Biletskyi wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29301/
> -----------------------------------------------------------
> 
> (Updated Jan. 14, 2015, 4:07 p.m.)
> 
> 
> Review request for kafka.
> 
> 
> Bugs: KAFKA-1694
>     https://issues.apache.org/jira/browse/KAFKA-1694
> 
> 
> Repository: kafka
> 
> 
> Description
> -------
> 
> KAFKA-1694 - introduced new type for Wire protocol, ported ClusterMetadataResponse to it
> 
> 
> KAFKA-1694 - Split Admin RQ/RP to separate messages
> 
> 
> KAFKA-1694 - Admin commands can be handled only by controller; DeleteTopicCommand NPE fix
> 
> 
> KAFKA-1776 - Ported ConsumerGroupOffsetChecker
> 
> 
> KAFKA-1776 - Ported PreferredReplicaElectionTool and ReassignPartitionsTool to CLI
> 
> 
> KAFKA-1694 - kafka-tools is uploaded on uploadAllArchives
> 
> 
> KAFKA-1694 - ReviewBoard 29301 code review fixes
> 
> 
> KAFKA-1694 - Data for ReassignPartitions and PreferredReplicaLeaderElection is in json string
> 
> 
> KAFKA-1694 - Added logging
> 
> 
> KAFKA-1694 - fixed misprint in schema
> 
> 
> KAFKA-1694 - DescribeTopicCommand supports all flags that TopicCommand does
> 
> 
> KAFKA-1694 - Fixed compile error for new Selector constructor
> 
> 
> KAFKA-1694 - Fixed ConsumerGroupChecker sends DescribeTopicResponse instead of ConsumerGroupOffsetsResponse
> 
> 
> KAFKA-1694 - Introduced AbstractAdminRequest/Response to avoid code duplication for sending admin requests / receiving response.
> 
> 
> Diffs
> -----
> 
>   bin/kafka.sh PRE-CREATION 
>   bin/windows/kafka.bat PRE-CREATION 
>   build.gradle c9ac43378c3bf5443f0f47c8ba76067237ecb348 
>   clients/src/main/java/org/apache/kafka/common/protocol/ApiKeys.java 109fc965e09b2ed186a073351bd037ac8af20a4c 
>   clients/src/main/java/org/apache/kafka/common/protocol/Protocol.java 7517b879866fc5dad5f8d8ad30636da8bbe7784a 
>   clients/src/main/java/org/apache/kafka/common/protocol/types/MaybeOf.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/protocol/types/Struct.java 121e880a941fcd3e6392859edba11a94236494cc 
>   clients/src/main/java/org/apache/kafka/common/requests/ClusterMetadataRequest.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/ClusterMetadataResponse.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/AbstractAdminRequest.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/AbstractAdminResponse.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/AlterTopicRequest.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/AlterTopicResponse.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/ConsumerGroupOffsetsRequest.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/ConsumerGroupOffsetsResponse.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/CreateTopicRequest.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/CreateTopicResponse.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/DeleteTopicRequest.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/DeleteTopicResponse.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/DescribeTopicOutput.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/DescribeTopicRequest.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/DescribeTopicResponse.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/ListTopicsOutput.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/ListTopicsRequest.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/ListTopicsResponse.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/PreferredReplicaLeaderElectionRequest.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/PreferredReplicaLeaderElectionResponse.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/ReassignPartitionsRequest.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/ReassignPartitionsResponse.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/TopicConfigDetails.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/TopicPartitionDetails.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/VerifyPreferredReplicaLeaderElectionRequest.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/VerifyPreferredReplicaLeaderElectionResponse.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/VerifyReassignPartitionsRequest.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/VerifyReassignPartitionsResponse.java PRE-CREATION 
>   clients/src/test/java/org/apache/kafka/common/requests/RequestResponseTest.java df37fc6d8f0db0b8192a948426af603be3444da4 
>   config/tools-log4j.properties 52f07c96019b4083fc78f62cfb0a81080327e847 
>   core/src/main/scala/kafka/api/ApiUtils.scala 1f80de1638978901500df808ca5133308c9d1fca 
>   core/src/main/scala/kafka/api/ClusterMetadataRequest.scala PRE-CREATION 
>   core/src/main/scala/kafka/api/ClusterMetadataResponse.scala PRE-CREATION 
>   core/src/main/scala/kafka/api/RequestKeys.scala c24c0345feedc7b9e2e9f40af11bfa1b8d328c43 
>   core/src/main/scala/kafka/api/admin/AlterTopicRequest.scala PRE-CREATION 
>   core/src/main/scala/kafka/api/admin/AlterTopicResponse.scala PRE-CREATION 
>   core/src/main/scala/kafka/api/admin/ConsumerGroupOffsetsRequest.scala PRE-CREATION 
>   core/src/main/scala/kafka/api/admin/ConsumerGroupOffsetsResponse.scala PRE-CREATION 
>   core/src/main/scala/kafka/api/admin/CreateTopicRequest.scala PRE-CREATION 
>   core/src/main/scala/kafka/api/admin/CreateTopicResponse.scala PRE-CREATION 
>   core/src/main/scala/kafka/api/admin/DeleteTopicRequest.scala PRE-CREATION 
>   core/src/main/scala/kafka/api/admin/DeleteTopicResponse.scala PRE-CREATION 
>   core/src/main/scala/kafka/api/admin/DescribeTopicRequest.scala PRE-CREATION 
>   core/src/main/scala/kafka/api/admin/DescribeTopicResponse.scala PRE-CREATION 
>   core/src/main/scala/kafka/api/admin/ListTopicsRequest.scala PRE-CREATION 
>   core/src/main/scala/kafka/api/admin/ListTopicsResponse.scala PRE-CREATION 
>   core/src/main/scala/kafka/api/admin/PreferredReplicaLeaderElectionRequest.scala PRE-CREATION 
>   core/src/main/scala/kafka/api/admin/PreferredReplicaLeaderElectionResponse.scala PRE-CREATION 
>   core/src/main/scala/kafka/api/admin/ReassignPartitionsRequest.scala PRE-CREATION 
>   core/src/main/scala/kafka/api/admin/ReassignPartitionsResponse.scala PRE-CREATION 
>   core/src/main/scala/kafka/api/admin/VerifyPreferredReplicaLeaderElectionRequest.scala PRE-CREATION 
>   core/src/main/scala/kafka/api/admin/VerifyPreferredReplicaLeaderElectionResponse.scala PRE-CREATION 
>   core/src/main/scala/kafka/api/admin/VerifyReassignPartitionsRequest.scala PRE-CREATION 
>   core/src/main/scala/kafka/api/admin/VerifyReassignPartitionsResponse.scala PRE-CREATION 
>   core/src/main/scala/kafka/common/AdminRequestFailedException.scala PRE-CREATION 
>   core/src/main/scala/kafka/common/ErrorMapping.scala eedc2f5f21dd8755fba891998456351622e17047 
>   core/src/main/scala/kafka/common/InvalidRequestTargetException.scala PRE-CREATION 
>   core/src/main/scala/kafka/controller/ControllerChannelManager.scala eb492f00449744bc8d63f55b393e2a1659d38454 
>   core/src/main/scala/kafka/controller/KafkaController.scala 66df6d2fbdbdd556da6bea0df84f93e0472c8fbf 
>   core/src/main/scala/kafka/server/KafkaApis.scala c011a1b79bd6c4e832fe7d097daacb0d647d1cd4 
>   core/src/main/scala/kafka/server/MetadataCache.scala bf81a1ab88c14be8697b441eedbeb28fa0112643 
>   core/src/main/scala/kafka/server/TopicCommandHelper.scala PRE-CREATION 
>   core/src/main/scala/kafka/tools/ConsumerOffsetCheckerHelper.scala PRE-CREATION 
>   core/src/main/scala/kafka/tools/PreferredReplicaLeaderElectionHelper.scala PRE-CREATION 
>   core/src/main/scala/kafka/tools/ReassignPartitionsHelper.scala PRE-CREATION 
>   core/src/test/scala/unit/kafka/api/RequestResponseSerializationTest.scala cd16ced5465d098be7a60498326b2a98c248f343 
>   settings.gradle 83f764e6a4a15a5fdba232dce74a369870f26b45 
>   tools/src/main/java/org/apache/kafka/cli/BaseCommandOpts.java PRE-CREATION 
>   tools/src/main/java/org/apache/kafka/cli/Boot.java PRE-CREATION 
>   tools/src/main/java/org/apache/kafka/cli/RequestDispatcher.java PRE-CREATION 
>   tools/src/main/java/org/apache/kafka/cli/Shell.java PRE-CREATION 
>   tools/src/main/java/org/apache/kafka/cli/command/AlterTopicCommand.java PRE-CREATION 
>   tools/src/main/java/org/apache/kafka/cli/command/ClearScreenCommand.java PRE-CREATION 
>   tools/src/main/java/org/apache/kafka/cli/command/Command.java PRE-CREATION 
>   tools/src/main/java/org/apache/kafka/cli/command/CreateTopicCommand.java PRE-CREATION 
>   tools/src/main/java/org/apache/kafka/cli/command/DeleteTopicCommand.java PRE-CREATION 
>   tools/src/main/java/org/apache/kafka/cli/command/DescribeTopicCommand.java PRE-CREATION 
>   tools/src/main/java/org/apache/kafka/cli/command/ExitCommand.java PRE-CREATION 
>   tools/src/main/java/org/apache/kafka/cli/command/ListTopicsCommand.java PRE-CREATION 
>   tools/src/main/java/org/apache/kafka/cli/command/PreferredReplicaLeaderElectionCommand.java PRE-CREATION 
>   tools/src/main/java/org/apache/kafka/cli/command/PrintConsumerGroupOffsetsCommand.java PRE-CREATION 
>   tools/src/main/java/org/apache/kafka/cli/command/PrintHelpCommand.java PRE-CREATION 
>   tools/src/main/java/org/apache/kafka/cli/command/ReassignPartitionsCommand.java PRE-CREATION 
>   tools/src/main/java/org/apache/kafka/cli/command/TopicSwitchCommand.java PRE-CREATION 
>   tools/src/main/java/org/apache/kafka/cli/util/StringUtils.java PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/29301/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Andrii Biletskyi
> 
>


Re: Review Request 29301: Patch for KAFKA-1694

Posted by Andrii Biletskyi <an...@stealth.ly>.

> On Feb. 3, 2015, 7:14 p.m., Guozhang Wang wrote:
> > core/src/main/scala/kafka/server/TopicCommandHelper.scala, lines 1-17
> > <https://reviews.apache.org/r/29301/diff/7/?file=821380#file821380line1>
> >
> >     One general comment:
> >     
> >     For some topic commands, why use AdminUtils to write ZK path again instead of handle it via the controller directly? Or this is still WIP?
> 
> Andrii Biletskyi wrote:
>     Not sure I understand you. You mean technially calling ZK client from Controller class, not through TopicCommandHelper? If so - it's just to leave KafkaApi clean and small.
> 
> Guozhang Wang wrote:
>     For example, upon receiving a create-topic request, the helper class will call AdminUtils.createOrUpdateTopicPartitionAssignmentPathInZK() which will just write this request to ZK admin path for it to be captured by controller; however since only the broker with the active controller will receive such requests why don't we just hand off the request from KafkaApi to the controller to handle it.
>     
>     One question, though, is that we need to make sure concurrency is correct for controller handling multiple such tasks, and we have some thoughts about how to deal with such cases (see Jiangjie and my commnets in KAFKA-1305).

Thanks for explanation.
So instead of current workflow:
CreateTopicRequest -> Helper class -> AdminUtils -> zk path is created -> Controller's changeTopicListener picks up the change -> topic is created
You propose:
CreateTopicRequest -> Controller directly executes logic from ChangeTopicListener
?
Very interesting idea! Can we make a separate ticket for that? I tried to port TopicCommand "as is" in order to have at least for now working end-to-end infrastructure to handle Admin commands. I believe this is more like refactoring TopicCommand (probably delete- and alterTopic should be changed too). I'm a bit concerned adding this refactoring will require additional efforts to test (especially taking into account your note about KAFKA-1305) and time to agree on approach we will use to address this issue.


> On Feb. 3, 2015, 7:14 p.m., Guozhang Wang wrote:
> > core/src/main/scala/kafka/controller/ControllerChannelManager.scala, lines 301-310
> > <https://reviews.apache.org/r/29301/diff/7/?file=821376#file821376line301>
> >
> >     Do not understand the rationale behind this: could you add some comments? Particularly, why we want to send an empty metadata map to the brokers with forceSendBrokerInfo?
> 
> Andrii Biletskyi wrote:
>     Thanks, this is done because on startup we don't send UpdateMetadaRequest (updateMetadataRequestMap is empty) and thus brokers' cache is not filled with brokers and controller. This leads to ClusterMetadataRequest can't be served correctly. 
>     I'm not sure this is the best way to do it, open for suggestions.
> 
> Guozhang Wang wrote:
>     In this case can we just use addUpdateMetadataRequestForBrokers() before calling sendRequestsToBrokers()?

If I understood correctly - addUpdateMetadataRequestForBrokers() is already called, it's just nothing is added to UpdateMetadata. The steps are the following:
1. One broker cluster is started (no topics)
2. KafkaController.onControllerFailover() is called
3. sendUpdateMetadataRequest()
4. addUpdateMetadataRequest(): updateMetadataRequest is created foreach controllerContext.partitionLeadershipInfo.keySet (which is empty)
5. sendRequestsToBrokers(): we send UpdateMetadata foreach broker from updateMetadataRequestMap (which is empty) -> broker holding a controller's role doesn't receive UpdateMetadataRequest

So essentially the problem is that UpdateMetadaRequest holds data about controller, brokers _and_ partitionState but we send UpdateMetadaRequest only if there is partitionState update to be sent.


> On Feb. 3, 2015, 7:14 p.m., Guozhang Wang wrote:
> > clients/src/main/java/org/apache/kafka/common/requests/admin/AbstractAdminRequest.java, lines 1-28
> > <https://reviews.apache.org/r/29301/diff/7/?file=821321#file821321line1>
> >
> >     Wondering if an abstract admin request is necessary, as it does not have many common interface functions.
> 
> Andrii Biletskyi wrote:
>     This is needed to avoid code dupliaction in admin clients. See RequestDispatcher for example.
>     You will need to call admin request and get response of that type. Having AbstractAdminRequest (specifically createResponseCounterpart) lets you have:
>     ```
>     public <T extends AbstractAdminResponse> T sendAdminRequest(AbstractAdminRequest<T> abstractRequest) throws Exception {
>     ```
>     Instead of sendCreateTopicRequest, sendAlter... etc. If there is a better and cleaner way to achive this - please let me know.
> 
> Guozhang Wang wrote:
>     I see. How about changing "sendAdminRequest(AbstractAdminRequest<T>)" to "sendRequest(ClientRequest)" and the caller like AlterTopicCommand.execute() will be:
>     
>     AlterTopicRequest alterTopicRequest = // create the request
>     ClientRequest request = new ClientRequest(new RequestSend(...) ...)
>     dispatcher.sendRequest(request)
>     
>     This way we are duplicating the second line here in every upper-level class, while saving the admin interface. I actually do not know which one is better..

Yes, but you will also need typed response. Let me continue your example:

AlterTopicRequest alterTopicRequest = // create the request
ClientRequest request = new ClientRequest(new RequestSend(...) ...)
__ClientResponse response = dispatcher.sendRequest(request, ApiKeys.ALTER_TOPIC)__
__AlterTopicResponse alterTopicResponse = new AlterTopicResponse(response.responseBody())__
alterTopicResponse.// now get what you need from typed response

And you will have this NetworkClient related Stuff (RequestSend, ClientRequest ...) everywhere in you client code. But it looks pretty strange you can't have generic method to send request and get immidiately response of the required type.

So really RequestDispatcher allready has sendRequest() as you suggest, with sendAdminRequest I tried to address issue with getting response counterpart. But I agree that solution might mislead people, so if doesn't worth it - I'm okay to remove intermediate AbstractAdminRequest/Response.


- Andrii


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29301/#review70790
-----------------------------------------------------------


On Jan. 14, 2015, 4:07 p.m., Andrii Biletskyi wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29301/
> -----------------------------------------------------------
> 
> (Updated Jan. 14, 2015, 4:07 p.m.)
> 
> 
> Review request for kafka.
> 
> 
> Bugs: KAFKA-1694
>     https://issues.apache.org/jira/browse/KAFKA-1694
> 
> 
> Repository: kafka
> 
> 
> Description
> -------
> 
> KAFKA-1694 - introduced new type for Wire protocol, ported ClusterMetadataResponse to it
> 
> 
> KAFKA-1694 - Split Admin RQ/RP to separate messages
> 
> 
> KAFKA-1694 - Admin commands can be handled only by controller; DeleteTopicCommand NPE fix
> 
> 
> KAFKA-1776 - Ported ConsumerGroupOffsetChecker
> 
> 
> KAFKA-1776 - Ported PreferredReplicaElectionTool and ReassignPartitionsTool to CLI
> 
> 
> KAFKA-1694 - kafka-tools is uploaded on uploadAllArchives
> 
> 
> KAFKA-1694 - ReviewBoard 29301 code review fixes
> 
> 
> KAFKA-1694 - Data for ReassignPartitions and PreferredReplicaLeaderElection is in json string
> 
> 
> KAFKA-1694 - Added logging
> 
> 
> KAFKA-1694 - fixed misprint in schema
> 
> 
> KAFKA-1694 - DescribeTopicCommand supports all flags that TopicCommand does
> 
> 
> KAFKA-1694 - Fixed compile error for new Selector constructor
> 
> 
> KAFKA-1694 - Fixed ConsumerGroupChecker sends DescribeTopicResponse instead of ConsumerGroupOffsetsResponse
> 
> 
> KAFKA-1694 - Introduced AbstractAdminRequest/Response to avoid code duplication for sending admin requests / receiving response.
> 
> 
> Diffs
> -----
> 
>   bin/kafka.sh PRE-CREATION 
>   bin/windows/kafka.bat PRE-CREATION 
>   build.gradle c9ac43378c3bf5443f0f47c8ba76067237ecb348 
>   clients/src/main/java/org/apache/kafka/common/protocol/ApiKeys.java 109fc965e09b2ed186a073351bd037ac8af20a4c 
>   clients/src/main/java/org/apache/kafka/common/protocol/Protocol.java 7517b879866fc5dad5f8d8ad30636da8bbe7784a 
>   clients/src/main/java/org/apache/kafka/common/protocol/types/MaybeOf.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/protocol/types/Struct.java 121e880a941fcd3e6392859edba11a94236494cc 
>   clients/src/main/java/org/apache/kafka/common/requests/ClusterMetadataRequest.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/ClusterMetadataResponse.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/AbstractAdminRequest.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/AbstractAdminResponse.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/AlterTopicRequest.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/AlterTopicResponse.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/ConsumerGroupOffsetsRequest.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/ConsumerGroupOffsetsResponse.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/CreateTopicRequest.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/CreateTopicResponse.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/DeleteTopicRequest.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/DeleteTopicResponse.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/DescribeTopicOutput.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/DescribeTopicRequest.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/DescribeTopicResponse.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/ListTopicsOutput.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/ListTopicsRequest.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/ListTopicsResponse.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/PreferredReplicaLeaderElectionRequest.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/PreferredReplicaLeaderElectionResponse.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/ReassignPartitionsRequest.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/ReassignPartitionsResponse.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/TopicConfigDetails.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/TopicPartitionDetails.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/VerifyPreferredReplicaLeaderElectionRequest.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/VerifyPreferredReplicaLeaderElectionResponse.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/VerifyReassignPartitionsRequest.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/VerifyReassignPartitionsResponse.java PRE-CREATION 
>   clients/src/test/java/org/apache/kafka/common/requests/RequestResponseTest.java df37fc6d8f0db0b8192a948426af603be3444da4 
>   config/tools-log4j.properties 52f07c96019b4083fc78f62cfb0a81080327e847 
>   core/src/main/scala/kafka/api/ApiUtils.scala 1f80de1638978901500df808ca5133308c9d1fca 
>   core/src/main/scala/kafka/api/ClusterMetadataRequest.scala PRE-CREATION 
>   core/src/main/scala/kafka/api/ClusterMetadataResponse.scala PRE-CREATION 
>   core/src/main/scala/kafka/api/RequestKeys.scala c24c0345feedc7b9e2e9f40af11bfa1b8d328c43 
>   core/src/main/scala/kafka/api/admin/AlterTopicRequest.scala PRE-CREATION 
>   core/src/main/scala/kafka/api/admin/AlterTopicResponse.scala PRE-CREATION 
>   core/src/main/scala/kafka/api/admin/ConsumerGroupOffsetsRequest.scala PRE-CREATION 
>   core/src/main/scala/kafka/api/admin/ConsumerGroupOffsetsResponse.scala PRE-CREATION 
>   core/src/main/scala/kafka/api/admin/CreateTopicRequest.scala PRE-CREATION 
>   core/src/main/scala/kafka/api/admin/CreateTopicResponse.scala PRE-CREATION 
>   core/src/main/scala/kafka/api/admin/DeleteTopicRequest.scala PRE-CREATION 
>   core/src/main/scala/kafka/api/admin/DeleteTopicResponse.scala PRE-CREATION 
>   core/src/main/scala/kafka/api/admin/DescribeTopicRequest.scala PRE-CREATION 
>   core/src/main/scala/kafka/api/admin/DescribeTopicResponse.scala PRE-CREATION 
>   core/src/main/scala/kafka/api/admin/ListTopicsRequest.scala PRE-CREATION 
>   core/src/main/scala/kafka/api/admin/ListTopicsResponse.scala PRE-CREATION 
>   core/src/main/scala/kafka/api/admin/PreferredReplicaLeaderElectionRequest.scala PRE-CREATION 
>   core/src/main/scala/kafka/api/admin/PreferredReplicaLeaderElectionResponse.scala PRE-CREATION 
>   core/src/main/scala/kafka/api/admin/ReassignPartitionsRequest.scala PRE-CREATION 
>   core/src/main/scala/kafka/api/admin/ReassignPartitionsResponse.scala PRE-CREATION 
>   core/src/main/scala/kafka/api/admin/VerifyPreferredReplicaLeaderElectionRequest.scala PRE-CREATION 
>   core/src/main/scala/kafka/api/admin/VerifyPreferredReplicaLeaderElectionResponse.scala PRE-CREATION 
>   core/src/main/scala/kafka/api/admin/VerifyReassignPartitionsRequest.scala PRE-CREATION 
>   core/src/main/scala/kafka/api/admin/VerifyReassignPartitionsResponse.scala PRE-CREATION 
>   core/src/main/scala/kafka/common/AdminRequestFailedException.scala PRE-CREATION 
>   core/src/main/scala/kafka/common/ErrorMapping.scala eedc2f5f21dd8755fba891998456351622e17047 
>   core/src/main/scala/kafka/common/InvalidRequestTargetException.scala PRE-CREATION 
>   core/src/main/scala/kafka/controller/ControllerChannelManager.scala eb492f00449744bc8d63f55b393e2a1659d38454 
>   core/src/main/scala/kafka/controller/KafkaController.scala 66df6d2fbdbdd556da6bea0df84f93e0472c8fbf 
>   core/src/main/scala/kafka/server/KafkaApis.scala c011a1b79bd6c4e832fe7d097daacb0d647d1cd4 
>   core/src/main/scala/kafka/server/MetadataCache.scala bf81a1ab88c14be8697b441eedbeb28fa0112643 
>   core/src/main/scala/kafka/server/TopicCommandHelper.scala PRE-CREATION 
>   core/src/main/scala/kafka/tools/ConsumerOffsetCheckerHelper.scala PRE-CREATION 
>   core/src/main/scala/kafka/tools/PreferredReplicaLeaderElectionHelper.scala PRE-CREATION 
>   core/src/main/scala/kafka/tools/ReassignPartitionsHelper.scala PRE-CREATION 
>   core/src/test/scala/unit/kafka/api/RequestResponseSerializationTest.scala cd16ced5465d098be7a60498326b2a98c248f343 
>   settings.gradle 83f764e6a4a15a5fdba232dce74a369870f26b45 
>   tools/src/main/java/org/apache/kafka/cli/BaseCommandOpts.java PRE-CREATION 
>   tools/src/main/java/org/apache/kafka/cli/Boot.java PRE-CREATION 
>   tools/src/main/java/org/apache/kafka/cli/RequestDispatcher.java PRE-CREATION 
>   tools/src/main/java/org/apache/kafka/cli/Shell.java PRE-CREATION 
>   tools/src/main/java/org/apache/kafka/cli/command/AlterTopicCommand.java PRE-CREATION 
>   tools/src/main/java/org/apache/kafka/cli/command/ClearScreenCommand.java PRE-CREATION 
>   tools/src/main/java/org/apache/kafka/cli/command/Command.java PRE-CREATION 
>   tools/src/main/java/org/apache/kafka/cli/command/CreateTopicCommand.java PRE-CREATION 
>   tools/src/main/java/org/apache/kafka/cli/command/DeleteTopicCommand.java PRE-CREATION 
>   tools/src/main/java/org/apache/kafka/cli/command/DescribeTopicCommand.java PRE-CREATION 
>   tools/src/main/java/org/apache/kafka/cli/command/ExitCommand.java PRE-CREATION 
>   tools/src/main/java/org/apache/kafka/cli/command/ListTopicsCommand.java PRE-CREATION 
>   tools/src/main/java/org/apache/kafka/cli/command/PreferredReplicaLeaderElectionCommand.java PRE-CREATION 
>   tools/src/main/java/org/apache/kafka/cli/command/PrintConsumerGroupOffsetsCommand.java PRE-CREATION 
>   tools/src/main/java/org/apache/kafka/cli/command/PrintHelpCommand.java PRE-CREATION 
>   tools/src/main/java/org/apache/kafka/cli/command/ReassignPartitionsCommand.java PRE-CREATION 
>   tools/src/main/java/org/apache/kafka/cli/command/TopicSwitchCommand.java PRE-CREATION 
>   tools/src/main/java/org/apache/kafka/cli/util/StringUtils.java PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/29301/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Andrii Biletskyi
> 
>


Re: Review Request 29301: Patch for KAFKA-1694

Posted by Andrii Biletskyi <an...@stealth.ly>.

> On Feb. 3, 2015, 7:14 p.m., Guozhang Wang wrote:
> > clients/src/main/java/org/apache/kafka/common/protocol/ApiKeys.java, lines 39-42
> > <https://reviews.apache.org/r/29301/diff/7/?file=821315#file821315line39>
> >
> >     How about merge them into one request? The format could be:
> >     
> >     topic -> string
> >     action -> string (create, alter, delete, describe)
> >     num_partition (?) -> int32
> >     reassignment (?) -> string
> >     configs (?) -> list[string]
> >     
> >     For configs, the client can first get the current config overriden list, and get the new configs list as (curr + added - deleted) so that we do not need two configs field in the request.

I see your point but I'm not sure this will comply Wire protocol use cases. My understanding is that Wire protocol has schema and RQ/RP message is always defined and static.
So it's probably possible to merge all requests in one but reusing some of the fields in different types and thus having them different meaning, makes such request type stand out from the others.
Also I believe merging all create-alter-delete-describe won't go that smoothly:
1) 'describe' has very different _response_ schema than "mutating" 'create-alter-delete'
2) I will probably remove MaybeOf type, as guys had concerns, this is different how empty values are handled now (see Jay's comment in KIP discussion) - this will make things even more tangled
3) There was a comment about batching commands - I'll probably modify schema so it will accept whitelist for topics (as in TopicCommand), but this is right only for 'alter-delete' - you can't create topic by reqexp - another issue to find the common point
4) There is also 'replicas' field which is used only in 'create'

As you see messages are really different.


> On Feb. 3, 2015, 7:14 p.m., Guozhang Wang wrote:
> > clients/src/main/java/org/apache/kafka/common/requests/admin/AbstractAdminRequest.java, lines 1-28
> > <https://reviews.apache.org/r/29301/diff/7/?file=821321#file821321line1>
> >
> >     Wondering if an abstract admin request is necessary, as it does not have many common interface functions.

This is needed to avoid code dupliaction in admin clients. See RequestDispatcher for example.
You will need to call admin request and get response of that type. Having AbstractAdminRequest (specifically createResponseCounterpart) lets you have:
```
public <T extends AbstractAdminResponse> T sendAdminRequest(AbstractAdminRequest<T> abstractRequest) throws Exception {
```
Instead of sendCreateTopicRequest, sendAlter... etc. If there is a better and cleaner way to achive this - please let me know.


> On Feb. 3, 2015, 7:14 p.m., Guozhang Wang wrote:
> > core/src/main/scala/kafka/server/TopicCommandHelper.scala, lines 1-17
> > <https://reviews.apache.org/r/29301/diff/7/?file=821380#file821380line1>
> >
> >     One general comment:
> >     
> >     For some topic commands, why use AdminUtils to write ZK path again instead of handle it via the controller directly? Or this is still WIP?

Not sure I understand you. You mean technially calling ZK client from Controller class, not through TopicCommandHelper? If so - it's just to leave KafkaApi clean and small.


> On Feb. 3, 2015, 7:14 p.m., Guozhang Wang wrote:
> > core/src/main/scala/kafka/controller/ControllerChannelManager.scala, lines 301-310
> > <https://reviews.apache.org/r/29301/diff/7/?file=821376#file821376line301>
> >
> >     Do not understand the rationale behind this: could you add some comments? Particularly, why we want to send an empty metadata map to the brokers with forceSendBrokerInfo?

Thanks, this is done because on startup we don't send UpdateMetadaRequest (updateMetadataRequestMap is empty) and thus brokers' cache is not filled with brokers and controller. This leads to ClusterMetadataRequest can't be served correctly. 
I'm not sure this is the best way to do it, open for suggestions.


- Andrii


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29301/#review70790
-----------------------------------------------------------


On Jan. 14, 2015, 4:07 p.m., Andrii Biletskyi wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29301/
> -----------------------------------------------------------
> 
> (Updated Jan. 14, 2015, 4:07 p.m.)
> 
> 
> Review request for kafka.
> 
> 
> Bugs: KAFKA-1694
>     https://issues.apache.org/jira/browse/KAFKA-1694
> 
> 
> Repository: kafka
> 
> 
> Description
> -------
> 
> KAFKA-1694 - introduced new type for Wire protocol, ported ClusterMetadataResponse to it
> 
> 
> KAFKA-1694 - Split Admin RQ/RP to separate messages
> 
> 
> KAFKA-1694 - Admin commands can be handled only by controller; DeleteTopicCommand NPE fix
> 
> 
> KAFKA-1776 - Ported ConsumerGroupOffsetChecker
> 
> 
> KAFKA-1776 - Ported PreferredReplicaElectionTool and ReassignPartitionsTool to CLI
> 
> 
> KAFKA-1694 - kafka-tools is uploaded on uploadAllArchives
> 
> 
> KAFKA-1694 - ReviewBoard 29301 code review fixes
> 
> 
> KAFKA-1694 - Data for ReassignPartitions and PreferredReplicaLeaderElection is in json string
> 
> 
> KAFKA-1694 - Added logging
> 
> 
> KAFKA-1694 - fixed misprint in schema
> 
> 
> KAFKA-1694 - DescribeTopicCommand supports all flags that TopicCommand does
> 
> 
> KAFKA-1694 - Fixed compile error for new Selector constructor
> 
> 
> KAFKA-1694 - Fixed ConsumerGroupChecker sends DescribeTopicResponse instead of ConsumerGroupOffsetsResponse
> 
> 
> KAFKA-1694 - Introduced AbstractAdminRequest/Response to avoid code duplication for sending admin requests / receiving response.
> 
> 
> Diffs
> -----
> 
>   bin/kafka.sh PRE-CREATION 
>   bin/windows/kafka.bat PRE-CREATION 
>   build.gradle c9ac43378c3bf5443f0f47c8ba76067237ecb348 
>   clients/src/main/java/org/apache/kafka/common/protocol/ApiKeys.java 109fc965e09b2ed186a073351bd037ac8af20a4c 
>   clients/src/main/java/org/apache/kafka/common/protocol/Protocol.java 7517b879866fc5dad5f8d8ad30636da8bbe7784a 
>   clients/src/main/java/org/apache/kafka/common/protocol/types/MaybeOf.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/protocol/types/Struct.java 121e880a941fcd3e6392859edba11a94236494cc 
>   clients/src/main/java/org/apache/kafka/common/requests/ClusterMetadataRequest.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/ClusterMetadataResponse.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/AbstractAdminRequest.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/AbstractAdminResponse.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/AlterTopicRequest.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/AlterTopicResponse.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/ConsumerGroupOffsetsRequest.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/ConsumerGroupOffsetsResponse.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/CreateTopicRequest.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/CreateTopicResponse.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/DeleteTopicRequest.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/DeleteTopicResponse.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/DescribeTopicOutput.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/DescribeTopicRequest.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/DescribeTopicResponse.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/ListTopicsOutput.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/ListTopicsRequest.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/ListTopicsResponse.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/PreferredReplicaLeaderElectionRequest.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/PreferredReplicaLeaderElectionResponse.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/ReassignPartitionsRequest.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/ReassignPartitionsResponse.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/TopicConfigDetails.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/TopicPartitionDetails.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/VerifyPreferredReplicaLeaderElectionRequest.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/VerifyPreferredReplicaLeaderElectionResponse.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/VerifyReassignPartitionsRequest.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/VerifyReassignPartitionsResponse.java PRE-CREATION 
>   clients/src/test/java/org/apache/kafka/common/requests/RequestResponseTest.java df37fc6d8f0db0b8192a948426af603be3444da4 
>   config/tools-log4j.properties 52f07c96019b4083fc78f62cfb0a81080327e847 
>   core/src/main/scala/kafka/api/ApiUtils.scala 1f80de1638978901500df808ca5133308c9d1fca 
>   core/src/main/scala/kafka/api/ClusterMetadataRequest.scala PRE-CREATION 
>   core/src/main/scala/kafka/api/ClusterMetadataResponse.scala PRE-CREATION 
>   core/src/main/scala/kafka/api/RequestKeys.scala c24c0345feedc7b9e2e9f40af11bfa1b8d328c43 
>   core/src/main/scala/kafka/api/admin/AlterTopicRequest.scala PRE-CREATION 
>   core/src/main/scala/kafka/api/admin/AlterTopicResponse.scala PRE-CREATION 
>   core/src/main/scala/kafka/api/admin/ConsumerGroupOffsetsRequest.scala PRE-CREATION 
>   core/src/main/scala/kafka/api/admin/ConsumerGroupOffsetsResponse.scala PRE-CREATION 
>   core/src/main/scala/kafka/api/admin/CreateTopicRequest.scala PRE-CREATION 
>   core/src/main/scala/kafka/api/admin/CreateTopicResponse.scala PRE-CREATION 
>   core/src/main/scala/kafka/api/admin/DeleteTopicRequest.scala PRE-CREATION 
>   core/src/main/scala/kafka/api/admin/DeleteTopicResponse.scala PRE-CREATION 
>   core/src/main/scala/kafka/api/admin/DescribeTopicRequest.scala PRE-CREATION 
>   core/src/main/scala/kafka/api/admin/DescribeTopicResponse.scala PRE-CREATION 
>   core/src/main/scala/kafka/api/admin/ListTopicsRequest.scala PRE-CREATION 
>   core/src/main/scala/kafka/api/admin/ListTopicsResponse.scala PRE-CREATION 
>   core/src/main/scala/kafka/api/admin/PreferredReplicaLeaderElectionRequest.scala PRE-CREATION 
>   core/src/main/scala/kafka/api/admin/PreferredReplicaLeaderElectionResponse.scala PRE-CREATION 
>   core/src/main/scala/kafka/api/admin/ReassignPartitionsRequest.scala PRE-CREATION 
>   core/src/main/scala/kafka/api/admin/ReassignPartitionsResponse.scala PRE-CREATION 
>   core/src/main/scala/kafka/api/admin/VerifyPreferredReplicaLeaderElectionRequest.scala PRE-CREATION 
>   core/src/main/scala/kafka/api/admin/VerifyPreferredReplicaLeaderElectionResponse.scala PRE-CREATION 
>   core/src/main/scala/kafka/api/admin/VerifyReassignPartitionsRequest.scala PRE-CREATION 
>   core/src/main/scala/kafka/api/admin/VerifyReassignPartitionsResponse.scala PRE-CREATION 
>   core/src/main/scala/kafka/common/AdminRequestFailedException.scala PRE-CREATION 
>   core/src/main/scala/kafka/common/ErrorMapping.scala eedc2f5f21dd8755fba891998456351622e17047 
>   core/src/main/scala/kafka/common/InvalidRequestTargetException.scala PRE-CREATION 
>   core/src/main/scala/kafka/controller/ControllerChannelManager.scala eb492f00449744bc8d63f55b393e2a1659d38454 
>   core/src/main/scala/kafka/controller/KafkaController.scala 66df6d2fbdbdd556da6bea0df84f93e0472c8fbf 
>   core/src/main/scala/kafka/server/KafkaApis.scala c011a1b79bd6c4e832fe7d097daacb0d647d1cd4 
>   core/src/main/scala/kafka/server/MetadataCache.scala bf81a1ab88c14be8697b441eedbeb28fa0112643 
>   core/src/main/scala/kafka/server/TopicCommandHelper.scala PRE-CREATION 
>   core/src/main/scala/kafka/tools/ConsumerOffsetCheckerHelper.scala PRE-CREATION 
>   core/src/main/scala/kafka/tools/PreferredReplicaLeaderElectionHelper.scala PRE-CREATION 
>   core/src/main/scala/kafka/tools/ReassignPartitionsHelper.scala PRE-CREATION 
>   core/src/test/scala/unit/kafka/api/RequestResponseSerializationTest.scala cd16ced5465d098be7a60498326b2a98c248f343 
>   settings.gradle 83f764e6a4a15a5fdba232dce74a369870f26b45 
>   tools/src/main/java/org/apache/kafka/cli/BaseCommandOpts.java PRE-CREATION 
>   tools/src/main/java/org/apache/kafka/cli/Boot.java PRE-CREATION 
>   tools/src/main/java/org/apache/kafka/cli/RequestDispatcher.java PRE-CREATION 
>   tools/src/main/java/org/apache/kafka/cli/Shell.java PRE-CREATION 
>   tools/src/main/java/org/apache/kafka/cli/command/AlterTopicCommand.java PRE-CREATION 
>   tools/src/main/java/org/apache/kafka/cli/command/ClearScreenCommand.java PRE-CREATION 
>   tools/src/main/java/org/apache/kafka/cli/command/Command.java PRE-CREATION 
>   tools/src/main/java/org/apache/kafka/cli/command/CreateTopicCommand.java PRE-CREATION 
>   tools/src/main/java/org/apache/kafka/cli/command/DeleteTopicCommand.java PRE-CREATION 
>   tools/src/main/java/org/apache/kafka/cli/command/DescribeTopicCommand.java PRE-CREATION 
>   tools/src/main/java/org/apache/kafka/cli/command/ExitCommand.java PRE-CREATION 
>   tools/src/main/java/org/apache/kafka/cli/command/ListTopicsCommand.java PRE-CREATION 
>   tools/src/main/java/org/apache/kafka/cli/command/PreferredReplicaLeaderElectionCommand.java PRE-CREATION 
>   tools/src/main/java/org/apache/kafka/cli/command/PrintConsumerGroupOffsetsCommand.java PRE-CREATION 
>   tools/src/main/java/org/apache/kafka/cli/command/PrintHelpCommand.java PRE-CREATION 
>   tools/src/main/java/org/apache/kafka/cli/command/ReassignPartitionsCommand.java PRE-CREATION 
>   tools/src/main/java/org/apache/kafka/cli/command/TopicSwitchCommand.java PRE-CREATION 
>   tools/src/main/java/org/apache/kafka/cli/util/StringUtils.java PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/29301/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Andrii Biletskyi
> 
>


Re: Review Request 29301: Patch for KAFKA-1694

Posted by Guozhang Wang <wa...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29301/#review70790
-----------------------------------------------------------


Looks good overall. Some comments below, which did not cover the client implementation and the some request helper classes.


build.gradle
<https://reviews.apache.org/r/29301/#comment116223>

    Why core compilation is dependent on tools compilation?



clients/src/main/java/org/apache/kafka/common/protocol/ApiKeys.java
<https://reviews.apache.org/r/29301/#comment116230>

    How about merge them into one request? The format could be:
    
    topic -> string
    action -> string (create, alter, delete, describe)
    num_partition (?) -> int32
    reassignment (?) -> string
    configs (?) -> list[string]
    
    For configs, the client can first get the current config overriden list, and get the new configs list as (curr + added - deleted) so that we do not need two configs field in the request.



clients/src/main/java/org/apache/kafka/common/protocol/ApiKeys.java
<https://reviews.apache.org/r/29301/#comment116225>

    How about just augmenting OFFSET_FETCH request to return offsets committed by others within the same group?



clients/src/main/java/org/apache/kafka/common/protocol/Protocol.java
<https://reviews.apache.org/r/29301/#comment116231>

    It seems we can replace with request with OFFSET_FETCH that can get all consumer offsets within the group. And the log-end-offset can be a separate and useful request by itself: we just need to combine these two requests to get the lag.



clients/src/main/java/org/apache/kafka/common/requests/ClusterMetadataResponse.java
<https://reviews.apache.org/r/29301/#comment116232>

    Rename to controllerStruct?



clients/src/main/java/org/apache/kafka/common/requests/admin/AbstractAdminRequest.java
<https://reviews.apache.org/r/29301/#comment116234>

    Wondering if an abstract admin request is necessary, as it does not have many common interface functions.



clients/src/main/java/org/apache/kafka/common/requests/admin/AbstractAdminResponse.java
<https://reviews.apache.org/r/29301/#comment116235>

    Ditto.



config/tools-log4j.properties
<https://reviews.apache.org/r/29301/#comment116236>

    Is this intentional? WARN => INFO



core/src/main/scala/kafka/api/ApiUtils.scala
<https://reviews.apache.org/r/29301/#comment116237>

    "reads a list of values of type T"



core/src/main/scala/kafka/api/ApiUtils.scala
<https://reviews.apache.org/r/29301/#comment116238>

    "reads a single value of type T"



core/src/main/scala/kafka/controller/ControllerChannelManager.scala
<https://reviews.apache.org/r/29301/#comment116239>

    Do not understand the rationale behind this: could you add some comments? Particularly, why we want to send an empty metadata map to the brokers with forceSendBrokerInfo?



core/src/main/scala/kafka/controller/KafkaController.scala
<https://reviews.apache.org/r/29301/#comment116240>

    Ditto above, would be better if some comments are added.



core/src/main/scala/kafka/server/TopicCommandHelper.scala
<https://reviews.apache.org/r/29301/#comment116246>

    One general comment:
    
    For some topic commands, why use AdminUtils to write ZK path again instead of handle it via the controller directly? Or this is still WIP?


- Guozhang Wang


On Jan. 14, 2015, 4:07 p.m., Andrii Biletskyi wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29301/
> -----------------------------------------------------------
> 
> (Updated Jan. 14, 2015, 4:07 p.m.)
> 
> 
> Review request for kafka.
> 
> 
> Bugs: KAFKA-1694
>     https://issues.apache.org/jira/browse/KAFKA-1694
> 
> 
> Repository: kafka
> 
> 
> Description
> -------
> 
> KAFKA-1694 - introduced new type for Wire protocol, ported ClusterMetadataResponse to it
> 
> 
> KAFKA-1694 - Split Admin RQ/RP to separate messages
> 
> 
> KAFKA-1694 - Admin commands can be handled only by controller; DeleteTopicCommand NPE fix
> 
> 
> KAFKA-1776 - Ported ConsumerGroupOffsetChecker
> 
> 
> KAFKA-1776 - Ported PreferredReplicaElectionTool and ReassignPartitionsTool to CLI
> 
> 
> KAFKA-1694 - kafka-tools is uploaded on uploadAllArchives
> 
> 
> KAFKA-1694 - ReviewBoard 29301 code review fixes
> 
> 
> KAFKA-1694 - Data for ReassignPartitions and PreferredReplicaLeaderElection is in json string
> 
> 
> KAFKA-1694 - Added logging
> 
> 
> KAFKA-1694 - fixed misprint in schema
> 
> 
> KAFKA-1694 - DescribeTopicCommand supports all flags that TopicCommand does
> 
> 
> KAFKA-1694 - Fixed compile error for new Selector constructor
> 
> 
> KAFKA-1694 - Fixed ConsumerGroupChecker sends DescribeTopicResponse instead of ConsumerGroupOffsetsResponse
> 
> 
> KAFKA-1694 - Introduced AbstractAdminRequest/Response to avoid code duplication for sending admin requests / receiving response.
> 
> 
> Diffs
> -----
> 
>   bin/kafka.sh PRE-CREATION 
>   bin/windows/kafka.bat PRE-CREATION 
>   build.gradle c9ac43378c3bf5443f0f47c8ba76067237ecb348 
>   clients/src/main/java/org/apache/kafka/common/protocol/ApiKeys.java 109fc965e09b2ed186a073351bd037ac8af20a4c 
>   clients/src/main/java/org/apache/kafka/common/protocol/Protocol.java 7517b879866fc5dad5f8d8ad30636da8bbe7784a 
>   clients/src/main/java/org/apache/kafka/common/protocol/types/MaybeOf.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/protocol/types/Struct.java 121e880a941fcd3e6392859edba11a94236494cc 
>   clients/src/main/java/org/apache/kafka/common/requests/ClusterMetadataRequest.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/ClusterMetadataResponse.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/AbstractAdminRequest.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/AbstractAdminResponse.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/AlterTopicRequest.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/AlterTopicResponse.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/ConsumerGroupOffsetsRequest.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/ConsumerGroupOffsetsResponse.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/CreateTopicRequest.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/CreateTopicResponse.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/DeleteTopicRequest.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/DeleteTopicResponse.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/DescribeTopicOutput.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/DescribeTopicRequest.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/DescribeTopicResponse.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/ListTopicsOutput.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/ListTopicsRequest.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/ListTopicsResponse.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/PreferredReplicaLeaderElectionRequest.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/PreferredReplicaLeaderElectionResponse.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/ReassignPartitionsRequest.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/ReassignPartitionsResponse.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/TopicConfigDetails.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/TopicPartitionDetails.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/VerifyPreferredReplicaLeaderElectionRequest.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/VerifyPreferredReplicaLeaderElectionResponse.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/VerifyReassignPartitionsRequest.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/VerifyReassignPartitionsResponse.java PRE-CREATION 
>   clients/src/test/java/org/apache/kafka/common/requests/RequestResponseTest.java df37fc6d8f0db0b8192a948426af603be3444da4 
>   config/tools-log4j.properties 52f07c96019b4083fc78f62cfb0a81080327e847 
>   core/src/main/scala/kafka/api/ApiUtils.scala 1f80de1638978901500df808ca5133308c9d1fca 
>   core/src/main/scala/kafka/api/ClusterMetadataRequest.scala PRE-CREATION 
>   core/src/main/scala/kafka/api/ClusterMetadataResponse.scala PRE-CREATION 
>   core/src/main/scala/kafka/api/RequestKeys.scala c24c0345feedc7b9e2e9f40af11bfa1b8d328c43 
>   core/src/main/scala/kafka/api/admin/AlterTopicRequest.scala PRE-CREATION 
>   core/src/main/scala/kafka/api/admin/AlterTopicResponse.scala PRE-CREATION 
>   core/src/main/scala/kafka/api/admin/ConsumerGroupOffsetsRequest.scala PRE-CREATION 
>   core/src/main/scala/kafka/api/admin/ConsumerGroupOffsetsResponse.scala PRE-CREATION 
>   core/src/main/scala/kafka/api/admin/CreateTopicRequest.scala PRE-CREATION 
>   core/src/main/scala/kafka/api/admin/CreateTopicResponse.scala PRE-CREATION 
>   core/src/main/scala/kafka/api/admin/DeleteTopicRequest.scala PRE-CREATION 
>   core/src/main/scala/kafka/api/admin/DeleteTopicResponse.scala PRE-CREATION 
>   core/src/main/scala/kafka/api/admin/DescribeTopicRequest.scala PRE-CREATION 
>   core/src/main/scala/kafka/api/admin/DescribeTopicResponse.scala PRE-CREATION 
>   core/src/main/scala/kafka/api/admin/ListTopicsRequest.scala PRE-CREATION 
>   core/src/main/scala/kafka/api/admin/ListTopicsResponse.scala PRE-CREATION 
>   core/src/main/scala/kafka/api/admin/PreferredReplicaLeaderElectionRequest.scala PRE-CREATION 
>   core/src/main/scala/kafka/api/admin/PreferredReplicaLeaderElectionResponse.scala PRE-CREATION 
>   core/src/main/scala/kafka/api/admin/ReassignPartitionsRequest.scala PRE-CREATION 
>   core/src/main/scala/kafka/api/admin/ReassignPartitionsResponse.scala PRE-CREATION 
>   core/src/main/scala/kafka/api/admin/VerifyPreferredReplicaLeaderElectionRequest.scala PRE-CREATION 
>   core/src/main/scala/kafka/api/admin/VerifyPreferredReplicaLeaderElectionResponse.scala PRE-CREATION 
>   core/src/main/scala/kafka/api/admin/VerifyReassignPartitionsRequest.scala PRE-CREATION 
>   core/src/main/scala/kafka/api/admin/VerifyReassignPartitionsResponse.scala PRE-CREATION 
>   core/src/main/scala/kafka/common/AdminRequestFailedException.scala PRE-CREATION 
>   core/src/main/scala/kafka/common/ErrorMapping.scala eedc2f5f21dd8755fba891998456351622e17047 
>   core/src/main/scala/kafka/common/InvalidRequestTargetException.scala PRE-CREATION 
>   core/src/main/scala/kafka/controller/ControllerChannelManager.scala eb492f00449744bc8d63f55b393e2a1659d38454 
>   core/src/main/scala/kafka/controller/KafkaController.scala 66df6d2fbdbdd556da6bea0df84f93e0472c8fbf 
>   core/src/main/scala/kafka/server/KafkaApis.scala c011a1b79bd6c4e832fe7d097daacb0d647d1cd4 
>   core/src/main/scala/kafka/server/MetadataCache.scala bf81a1ab88c14be8697b441eedbeb28fa0112643 
>   core/src/main/scala/kafka/server/TopicCommandHelper.scala PRE-CREATION 
>   core/src/main/scala/kafka/tools/ConsumerOffsetCheckerHelper.scala PRE-CREATION 
>   core/src/main/scala/kafka/tools/PreferredReplicaLeaderElectionHelper.scala PRE-CREATION 
>   core/src/main/scala/kafka/tools/ReassignPartitionsHelper.scala PRE-CREATION 
>   core/src/test/scala/unit/kafka/api/RequestResponseSerializationTest.scala cd16ced5465d098be7a60498326b2a98c248f343 
>   settings.gradle 83f764e6a4a15a5fdba232dce74a369870f26b45 
>   tools/src/main/java/org/apache/kafka/cli/BaseCommandOpts.java PRE-CREATION 
>   tools/src/main/java/org/apache/kafka/cli/Boot.java PRE-CREATION 
>   tools/src/main/java/org/apache/kafka/cli/RequestDispatcher.java PRE-CREATION 
>   tools/src/main/java/org/apache/kafka/cli/Shell.java PRE-CREATION 
>   tools/src/main/java/org/apache/kafka/cli/command/AlterTopicCommand.java PRE-CREATION 
>   tools/src/main/java/org/apache/kafka/cli/command/ClearScreenCommand.java PRE-CREATION 
>   tools/src/main/java/org/apache/kafka/cli/command/Command.java PRE-CREATION 
>   tools/src/main/java/org/apache/kafka/cli/command/CreateTopicCommand.java PRE-CREATION 
>   tools/src/main/java/org/apache/kafka/cli/command/DeleteTopicCommand.java PRE-CREATION 
>   tools/src/main/java/org/apache/kafka/cli/command/DescribeTopicCommand.java PRE-CREATION 
>   tools/src/main/java/org/apache/kafka/cli/command/ExitCommand.java PRE-CREATION 
>   tools/src/main/java/org/apache/kafka/cli/command/ListTopicsCommand.java PRE-CREATION 
>   tools/src/main/java/org/apache/kafka/cli/command/PreferredReplicaLeaderElectionCommand.java PRE-CREATION 
>   tools/src/main/java/org/apache/kafka/cli/command/PrintConsumerGroupOffsetsCommand.java PRE-CREATION 
>   tools/src/main/java/org/apache/kafka/cli/command/PrintHelpCommand.java PRE-CREATION 
>   tools/src/main/java/org/apache/kafka/cli/command/ReassignPartitionsCommand.java PRE-CREATION 
>   tools/src/main/java/org/apache/kafka/cli/command/TopicSwitchCommand.java PRE-CREATION 
>   tools/src/main/java/org/apache/kafka/cli/util/StringUtils.java PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/29301/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Andrii Biletskyi
> 
>


Re: Review Request 29301: Patch for KAFKA-1694

Posted by Andrii Biletskyi <an...@stealth.ly>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29301/
-----------------------------------------------------------

(Updated March 12, 2015, 11:04 a.m.)


Review request for kafka.


Bugs: KAFKA-1694
    https://issues.apache.org/jira/browse/KAFKA-1694


Repository: kafka


Description (updated)
-------

KAFKA-1694 - introduced new type for Wire protocol, ported ClusterMetadataResponse to it


KAFKA-1694 - Split Admin RQ/RP to separate messages


KAFKA-1694 - Admin commands can be handled only by controller; DeleteTopicCommand NPE fix


KAFKA-1776 - Ported ConsumerGroupOffsetChecker


KAFKA-1776 - Ported PreferredReplicaElectionTool and ReassignPartitionsTool to CLI


KAFKA-1694 - ReviewBoard 29301 code review fixes


KAFKA-1694 - Data for ReassignPartitions and PreferredReplicaLeaderElection is in json string


KAFKA-1694 - Added logging


KAFKA-1694 - fixed misprint in schema


KAFKA-1694 - DescribeTopicCommand supports all flags that TopicCommand does


KAFKA-1694 - Fixed compile error for new Selector constructor


KAFKA-1694 - Fixed ConsumerGroupChecker sends DescribeTopicResponse instead of ConsumerGroupOffsetsResponse


KAFKA-1694 - Introduced AbstractAdminRequest/Response to avoid code duplication for sending admin requests / receiving response.


KAFKA-1694 - Code review fix: /core shouldn't depend on /tools


KAFKA-1694 - Code review fix: normalized config field in Create- and AlterTopicRequest


KAFKA-1694 - Remove server RQ/RP messages, clients' classes are used instead


KAFKA-1694 - Remove ConsumerGroupOffsets RQ/RP - a separate KIP will be created


KAFKA-1694 - Remove MaybeOf type, clean up dead code


KAFKA-1694 - Post rebase merge conflicts fixes


KAFKA-1694 - Added Protocol errors


KAFKA-1694 - Bugfix - incorrect error


Diffs (updated)
-----

  bin/kafka.sh PRE-CREATION 
  bin/windows/kafka.bat PRE-CREATION 
  build.gradle 0f0fe60a74542efa91a0e727146e896edcaa38af 
  checkstyle/import-control.xml cca4b38ec766028a604f88a1c63228e40df24573 
  clients/src/main/java/org/apache/kafka/common/ConfigEntry.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/protocol/ApiKeys.java 07aba71303bc1303dbe05e4b121f73f7ad27fdb5 
  clients/src/main/java/org/apache/kafka/common/protocol/Errors.java ce18a6ce7ed31420cdbec6926a9cd04fa4c806b1 
  clients/src/main/java/org/apache/kafka/common/protocol/Protocol.java 101f382170ad6740b3f8ff2d27b93a64874a857f 
  clients/src/main/java/org/apache/kafka/common/requests/ClusterMetadataRequest.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/requests/ClusterMetadataResponse.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/requests/admin/AbstractAdminRequest.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/requests/admin/AbstractAdminResponse.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/requests/admin/AlterTopicRequest.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/requests/admin/AlterTopicResponse.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/requests/admin/CreateTopicRequest.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/requests/admin/CreateTopicResponse.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/requests/admin/DeleteTopicRequest.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/requests/admin/DeleteTopicResponse.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/requests/admin/DescribeTopicOutput.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/requests/admin/DescribeTopicRequest.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/requests/admin/DescribeTopicResponse.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/requests/admin/ListTopicsRequest.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/requests/admin/ListTopicsResponse.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/requests/admin/PreferredReplicaLeaderElectionRequest.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/requests/admin/PreferredReplicaLeaderElectionResponse.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/requests/admin/ReassignPartitionsRequest.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/requests/admin/ReassignPartitionsResponse.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/requests/admin/TopicConfigDetails.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/requests/admin/TopicPartitionDetails.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/requests/admin/VerifyReassignPartitionsRequest.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/requests/admin/VerifyReassignPartitionsResponse.java PRE-CREATION 
  clients/src/test/java/org/apache/kafka/common/requests/RequestResponseTest.java 13237fd72da5448a3d596b882fef141f336f827d 
  config/tools-log4j.properties 52f07c96019b4083fc78f62cfb0a81080327e847 
  core/src/main/scala/kafka/api/ApiUtils.scala 1f80de1638978901500df808ca5133308c9d1fca 
  core/src/main/scala/kafka/api/ClusterMetadaRequestAndHeader.scala PRE-CREATION 
  core/src/main/scala/kafka/api/ClusterMetadataResponseAndHeader.scala PRE-CREATION 
  core/src/main/scala/kafka/api/RequestKeys.scala c24c0345feedc7b9e2e9f40af11bfa1b8d328c43 
  core/src/main/scala/kafka/api/admin/AlterTopicRequestAndHeader.scala PRE-CREATION 
  core/src/main/scala/kafka/api/admin/AlterTopicResponseAndHeader.scala PRE-CREATION 
  core/src/main/scala/kafka/api/admin/CreateTopicRequestAndHeader.scala PRE-CREATION 
  core/src/main/scala/kafka/api/admin/CreateTopicResponseAndHeader.scala PRE-CREATION 
  core/src/main/scala/kafka/api/admin/DeleteTopicRequestAndHeader.scala PRE-CREATION 
  core/src/main/scala/kafka/api/admin/DeleteTopicResponseAndHeader.scala PRE-CREATION 
  core/src/main/scala/kafka/api/admin/DescribeTopicRequestAndHeader.scala PRE-CREATION 
  core/src/main/scala/kafka/api/admin/DescribeTopicResponseAndHeader.scala PRE-CREATION 
  core/src/main/scala/kafka/api/admin/ListTopicsRequestAndHeader.scala PRE-CREATION 
  core/src/main/scala/kafka/api/admin/ListTopicsResponseAndHeader.scala PRE-CREATION 
  core/src/main/scala/kafka/api/admin/PreferredReplicaLeaderElectionRequestAndHeader.scala PRE-CREATION 
  core/src/main/scala/kafka/api/admin/PreferredReplicaLeaderElectionResponseAndHeader.scala PRE-CREATION 
  core/src/main/scala/kafka/api/admin/ReassignPartitionsRequestAndHeader.scala PRE-CREATION 
  core/src/main/scala/kafka/api/admin/ReassignPartitionsResponseAndHeader.scala PRE-CREATION 
  core/src/main/scala/kafka/api/admin/VerifyReassignPartitionsRequestAndHeader.scala PRE-CREATION 
  core/src/main/scala/kafka/api/admin/VerifyReassignPartitionsResponseAndHeader.scala PRE-CREATION 
  core/src/main/scala/kafka/common/AdminRequestFailedException.scala PRE-CREATION 
  core/src/main/scala/kafka/common/ErrorMapping.scala eb1eb4a703098253d0aae79577084569177768d1 
  core/src/main/scala/kafka/common/NotControllerReceivedAdminRequestException.scala PRE-CREATION 
  core/src/main/scala/kafka/controller/ControllerChannelManager.scala c582191636f6188c25d62a67ff0315b56f163133 
  core/src/main/scala/kafka/server/KafkaApis.scala 35af98f0bc1b6a50bd1d97a30147593f8c6a422d 
  core/src/main/scala/kafka/server/MetadataCache.scala 6aef6e4508ecadbbcc1e12bed2054547b7aa333e 
  core/src/main/scala/kafka/server/TopicCommandHelper.scala PRE-CREATION 
  core/src/main/scala/kafka/tools/PreferredReplicaLeaderElectionHelper.scala PRE-CREATION 
  core/src/main/scala/kafka/tools/ReassignPartitionsHelper.scala PRE-CREATION 
  core/src/test/scala/unit/kafka/api/RequestResponseSerializationTest.scala fba852afa1b2f46b61e2fd12c38c821ba04e9cc6 
  settings.gradle 83f764e6a4a15a5fdba232dce74a369870f26b45 
  tools/src/main/java/org/apache/kafka/cli/BaseCommandOpts.java PRE-CREATION 
  tools/src/main/java/org/apache/kafka/cli/Boot.java PRE-CREATION 
  tools/src/main/java/org/apache/kafka/cli/RequestDispatcher.java PRE-CREATION 
  tools/src/main/java/org/apache/kafka/cli/Shell.java PRE-CREATION 
  tools/src/main/java/org/apache/kafka/cli/command/AlterTopicCommand.java PRE-CREATION 
  tools/src/main/java/org/apache/kafka/cli/command/ClearScreenCommand.java PRE-CREATION 
  tools/src/main/java/org/apache/kafka/cli/command/Command.java PRE-CREATION 
  tools/src/main/java/org/apache/kafka/cli/command/CreateTopicCommand.java PRE-CREATION 
  tools/src/main/java/org/apache/kafka/cli/command/DeleteTopicCommand.java PRE-CREATION 
  tools/src/main/java/org/apache/kafka/cli/command/DescribeTopicCommand.java PRE-CREATION 
  tools/src/main/java/org/apache/kafka/cli/command/ExitCommand.java PRE-CREATION 
  tools/src/main/java/org/apache/kafka/cli/command/ListTopicsCommand.java PRE-CREATION 
  tools/src/main/java/org/apache/kafka/cli/command/PreferredReplicaLeaderElectionCommand.java PRE-CREATION 
  tools/src/main/java/org/apache/kafka/cli/command/PrintHelpCommand.java PRE-CREATION 
  tools/src/main/java/org/apache/kafka/cli/command/ReassignPartitionsCommand.java PRE-CREATION 
  tools/src/main/java/org/apache/kafka/cli/command/TopicSwitchCommand.java PRE-CREATION 
  tools/src/main/java/org/apache/kafka/cli/util/StringUtils.java PRE-CREATION 

Diff: https://reviews.apache.org/r/29301/diff/


Testing
-------


Thanks,

Andrii Biletskyi


Re: Review Request 29301: Patch for KAFKA-1694

Posted by Andrii Biletskyi <an...@stealth.ly>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29301/
-----------------------------------------------------------

(Updated Jan. 14, 2015, 4:07 p.m.)


Review request for kafka.


Bugs: KAFKA-1694
    https://issues.apache.org/jira/browse/KAFKA-1694


Repository: kafka


Description (updated)
-------

KAFKA-1694 - introduced new type for Wire protocol, ported ClusterMetadataResponse to it


KAFKA-1694 - Split Admin RQ/RP to separate messages


KAFKA-1694 - Admin commands can be handled only by controller; DeleteTopicCommand NPE fix


KAFKA-1776 - Ported ConsumerGroupOffsetChecker


KAFKA-1776 - Ported PreferredReplicaElectionTool and ReassignPartitionsTool to CLI


KAFKA-1694 - kafka-tools is uploaded on uploadAllArchives


KAFKA-1694 - ReviewBoard 29301 code review fixes


KAFKA-1694 - Data for ReassignPartitions and PreferredReplicaLeaderElection is in json string


KAFKA-1694 - Added logging


KAFKA-1694 - fixed misprint in schema


KAFKA-1694 - DescribeTopicCommand supports all flags that TopicCommand does


KAFKA-1694 - Fixed compile error for new Selector constructor


KAFKA-1694 - Fixed ConsumerGroupChecker sends DescribeTopicResponse instead of ConsumerGroupOffsetsResponse


KAFKA-1694 - Introduced AbstractAdminRequest/Response to avoid code duplication for sending admin requests / receiving response.


Diffs (updated)
-----

  bin/kafka.sh PRE-CREATION 
  bin/windows/kafka.bat PRE-CREATION 
  build.gradle c9ac43378c3bf5443f0f47c8ba76067237ecb348 
  clients/src/main/java/org/apache/kafka/common/protocol/ApiKeys.java 109fc965e09b2ed186a073351bd037ac8af20a4c 
  clients/src/main/java/org/apache/kafka/common/protocol/Protocol.java 7517b879866fc5dad5f8d8ad30636da8bbe7784a 
  clients/src/main/java/org/apache/kafka/common/protocol/types/MaybeOf.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/protocol/types/Struct.java 121e880a941fcd3e6392859edba11a94236494cc 
  clients/src/main/java/org/apache/kafka/common/requests/ClusterMetadataRequest.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/requests/ClusterMetadataResponse.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/requests/admin/AbstractAdminRequest.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/requests/admin/AbstractAdminResponse.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/requests/admin/AlterTopicRequest.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/requests/admin/AlterTopicResponse.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/requests/admin/ConsumerGroupOffsetsRequest.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/requests/admin/ConsumerGroupOffsetsResponse.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/requests/admin/CreateTopicRequest.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/requests/admin/CreateTopicResponse.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/requests/admin/DeleteTopicRequest.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/requests/admin/DeleteTopicResponse.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/requests/admin/DescribeTopicOutput.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/requests/admin/DescribeTopicRequest.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/requests/admin/DescribeTopicResponse.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/requests/admin/ListTopicsOutput.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/requests/admin/ListTopicsRequest.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/requests/admin/ListTopicsResponse.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/requests/admin/PreferredReplicaLeaderElectionRequest.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/requests/admin/PreferredReplicaLeaderElectionResponse.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/requests/admin/ReassignPartitionsRequest.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/requests/admin/ReassignPartitionsResponse.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/requests/admin/TopicConfigDetails.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/requests/admin/TopicPartitionDetails.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/requests/admin/VerifyPreferredReplicaLeaderElectionRequest.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/requests/admin/VerifyPreferredReplicaLeaderElectionResponse.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/requests/admin/VerifyReassignPartitionsRequest.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/requests/admin/VerifyReassignPartitionsResponse.java PRE-CREATION 
  clients/src/test/java/org/apache/kafka/common/requests/RequestResponseTest.java df37fc6d8f0db0b8192a948426af603be3444da4 
  config/tools-log4j.properties 52f07c96019b4083fc78f62cfb0a81080327e847 
  core/src/main/scala/kafka/api/ApiUtils.scala 1f80de1638978901500df808ca5133308c9d1fca 
  core/src/main/scala/kafka/api/ClusterMetadataRequest.scala PRE-CREATION 
  core/src/main/scala/kafka/api/ClusterMetadataResponse.scala PRE-CREATION 
  core/src/main/scala/kafka/api/RequestKeys.scala c24c0345feedc7b9e2e9f40af11bfa1b8d328c43 
  core/src/main/scala/kafka/api/admin/AlterTopicRequest.scala PRE-CREATION 
  core/src/main/scala/kafka/api/admin/AlterTopicResponse.scala PRE-CREATION 
  core/src/main/scala/kafka/api/admin/ConsumerGroupOffsetsRequest.scala PRE-CREATION 
  core/src/main/scala/kafka/api/admin/ConsumerGroupOffsetsResponse.scala PRE-CREATION 
  core/src/main/scala/kafka/api/admin/CreateTopicRequest.scala PRE-CREATION 
  core/src/main/scala/kafka/api/admin/CreateTopicResponse.scala PRE-CREATION 
  core/src/main/scala/kafka/api/admin/DeleteTopicRequest.scala PRE-CREATION 
  core/src/main/scala/kafka/api/admin/DeleteTopicResponse.scala PRE-CREATION 
  core/src/main/scala/kafka/api/admin/DescribeTopicRequest.scala PRE-CREATION 
  core/src/main/scala/kafka/api/admin/DescribeTopicResponse.scala PRE-CREATION 
  core/src/main/scala/kafka/api/admin/ListTopicsRequest.scala PRE-CREATION 
  core/src/main/scala/kafka/api/admin/ListTopicsResponse.scala PRE-CREATION 
  core/src/main/scala/kafka/api/admin/PreferredReplicaLeaderElectionRequest.scala PRE-CREATION 
  core/src/main/scala/kafka/api/admin/PreferredReplicaLeaderElectionResponse.scala PRE-CREATION 
  core/src/main/scala/kafka/api/admin/ReassignPartitionsRequest.scala PRE-CREATION 
  core/src/main/scala/kafka/api/admin/ReassignPartitionsResponse.scala PRE-CREATION 
  core/src/main/scala/kafka/api/admin/VerifyPreferredReplicaLeaderElectionRequest.scala PRE-CREATION 
  core/src/main/scala/kafka/api/admin/VerifyPreferredReplicaLeaderElectionResponse.scala PRE-CREATION 
  core/src/main/scala/kafka/api/admin/VerifyReassignPartitionsRequest.scala PRE-CREATION 
  core/src/main/scala/kafka/api/admin/VerifyReassignPartitionsResponse.scala PRE-CREATION 
  core/src/main/scala/kafka/common/AdminRequestFailedException.scala PRE-CREATION 
  core/src/main/scala/kafka/common/ErrorMapping.scala eedc2f5f21dd8755fba891998456351622e17047 
  core/src/main/scala/kafka/common/InvalidRequestTargetException.scala PRE-CREATION 
  core/src/main/scala/kafka/controller/ControllerChannelManager.scala eb492f00449744bc8d63f55b393e2a1659d38454 
  core/src/main/scala/kafka/controller/KafkaController.scala 66df6d2fbdbdd556da6bea0df84f93e0472c8fbf 
  core/src/main/scala/kafka/server/KafkaApis.scala c011a1b79bd6c4e832fe7d097daacb0d647d1cd4 
  core/src/main/scala/kafka/server/MetadataCache.scala bf81a1ab88c14be8697b441eedbeb28fa0112643 
  core/src/main/scala/kafka/server/TopicCommandHelper.scala PRE-CREATION 
  core/src/main/scala/kafka/tools/ConsumerOffsetCheckerHelper.scala PRE-CREATION 
  core/src/main/scala/kafka/tools/PreferredReplicaLeaderElectionHelper.scala PRE-CREATION 
  core/src/main/scala/kafka/tools/ReassignPartitionsHelper.scala PRE-CREATION 
  core/src/test/scala/unit/kafka/api/RequestResponseSerializationTest.scala cd16ced5465d098be7a60498326b2a98c248f343 
  settings.gradle 83f764e6a4a15a5fdba232dce74a369870f26b45 
  tools/src/main/java/org/apache/kafka/cli/BaseCommandOpts.java PRE-CREATION 
  tools/src/main/java/org/apache/kafka/cli/Boot.java PRE-CREATION 
  tools/src/main/java/org/apache/kafka/cli/RequestDispatcher.java PRE-CREATION 
  tools/src/main/java/org/apache/kafka/cli/Shell.java PRE-CREATION 
  tools/src/main/java/org/apache/kafka/cli/command/AlterTopicCommand.java PRE-CREATION 
  tools/src/main/java/org/apache/kafka/cli/command/ClearScreenCommand.java PRE-CREATION 
  tools/src/main/java/org/apache/kafka/cli/command/Command.java PRE-CREATION 
  tools/src/main/java/org/apache/kafka/cli/command/CreateTopicCommand.java PRE-CREATION 
  tools/src/main/java/org/apache/kafka/cli/command/DeleteTopicCommand.java PRE-CREATION 
  tools/src/main/java/org/apache/kafka/cli/command/DescribeTopicCommand.java PRE-CREATION 
  tools/src/main/java/org/apache/kafka/cli/command/ExitCommand.java PRE-CREATION 
  tools/src/main/java/org/apache/kafka/cli/command/ListTopicsCommand.java PRE-CREATION 
  tools/src/main/java/org/apache/kafka/cli/command/PreferredReplicaLeaderElectionCommand.java PRE-CREATION 
  tools/src/main/java/org/apache/kafka/cli/command/PrintConsumerGroupOffsetsCommand.java PRE-CREATION 
  tools/src/main/java/org/apache/kafka/cli/command/PrintHelpCommand.java PRE-CREATION 
  tools/src/main/java/org/apache/kafka/cli/command/ReassignPartitionsCommand.java PRE-CREATION 
  tools/src/main/java/org/apache/kafka/cli/command/TopicSwitchCommand.java PRE-CREATION 
  tools/src/main/java/org/apache/kafka/cli/util/StringUtils.java PRE-CREATION 

Diff: https://reviews.apache.org/r/29301/diff/


Testing
-------


Thanks,

Andrii Biletskyi


Re: Review Request 29301: Patch for KAFKA-1694

Posted by Andrii Biletskyi <an...@stealth.ly>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29301/
-----------------------------------------------------------

(Updated Jan. 14, 2015, 1:42 p.m.)


Review request for kafka.


Bugs: KAFKA-1694
    https://issues.apache.org/jira/browse/KAFKA-1694


Repository: kafka


Description (updated)
-------

KAFKA-1694 - introduced new type for Wire protocol, ported ClusterMetadataResponse to it


KAFKA-1694 - Split Admin RQ/RP to separate messages


KAFKA-1694 - Admin commands can be handled only by controller; DeleteTopicCommand NPE fix


KAFKA-1776 - Ported ConsumerGroupOffsetChecker


KAFKA-1776 - Ported PreferredReplicaElectionTool and ReassignPartitionsTool to CLI


KAFKA-1694 - kafka-tools is uploaded on uploadAllArchives


KAFKA-1694 - ReviewBoard 29301 code review fixes


KAFKA-1694 - Data for ReassignPartitions and PreferredReplicaLeaderElection is in json string


KAFKA-1694 - Added logging


KAFKA-1694 - fixed misprint in schema


KAFKA-1694 - DescribeTopicCommand supports all flags that TopicCommand does


Diffs (updated)
-----

  bin/kafka.sh PRE-CREATION 
  bin/windows/kafka.bat PRE-CREATION 
  build.gradle c9ac43378c3bf5443f0f47c8ba76067237ecb348 
  clients/src/main/java/org/apache/kafka/common/protocol/ApiKeys.java 109fc965e09b2ed186a073351bd037ac8af20a4c 
  clients/src/main/java/org/apache/kafka/common/protocol/Protocol.java 7517b879866fc5dad5f8d8ad30636da8bbe7784a 
  clients/src/main/java/org/apache/kafka/common/protocol/types/MaybeOf.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/protocol/types/Struct.java 121e880a941fcd3e6392859edba11a94236494cc 
  clients/src/main/java/org/apache/kafka/common/requests/ClusterMetadataRequest.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/requests/ClusterMetadataResponse.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/requests/admin/AlterTopicRequest.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/requests/admin/AlterTopicResponse.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/requests/admin/ConsumerGroupOffsetsRequest.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/requests/admin/ConsumerGroupOffsetsResponse.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/requests/admin/CreateTopicRequest.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/requests/admin/CreateTopicResponse.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/requests/admin/DeleteTopicRequest.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/requests/admin/DeleteTopicResponse.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/requests/admin/DescribeTopicOutput.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/requests/admin/DescribeTopicRequest.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/requests/admin/DescribeTopicResponse.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/requests/admin/ListTopicsOutput.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/requests/admin/ListTopicsRequest.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/requests/admin/ListTopicsResponse.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/requests/admin/PreferredReplicaLeaderElectionRequest.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/requests/admin/PreferredReplicaLeaderElectionResponse.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/requests/admin/ReassignPartitionsRequest.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/requests/admin/ReassignPartitionsResponse.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/requests/admin/TopicConfigDetails.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/requests/admin/TopicPartitionDetails.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/requests/admin/VerifyPreferredReplicaLeaderElectionRequest.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/requests/admin/VerifyPreferredReplicaLeaderElectionResponse.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/requests/admin/VerifyReassignPartitionsRequest.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/requests/admin/VerifyReassignPartitionsResponse.java PRE-CREATION 
  clients/src/test/java/org/apache/kafka/common/requests/RequestResponseTest.java df37fc6d8f0db0b8192a948426af603be3444da4 
  config/tools-log4j.properties 52f07c96019b4083fc78f62cfb0a81080327e847 
  core/src/main/scala/kafka/api/ApiUtils.scala 1f80de1638978901500df808ca5133308c9d1fca 
  core/src/main/scala/kafka/api/ClusterMetadataRequest.scala PRE-CREATION 
  core/src/main/scala/kafka/api/ClusterMetadataResponse.scala PRE-CREATION 
  core/src/main/scala/kafka/api/RequestKeys.scala c24c0345feedc7b9e2e9f40af11bfa1b8d328c43 
  core/src/main/scala/kafka/api/admin/AlterTopicRequest.scala PRE-CREATION 
  core/src/main/scala/kafka/api/admin/AlterTopicResponse.scala PRE-CREATION 
  core/src/main/scala/kafka/api/admin/ConsumerGroupOffsetsRequest.scala PRE-CREATION 
  core/src/main/scala/kafka/api/admin/ConsumerGroupOffsetsResponse.scala PRE-CREATION 
  core/src/main/scala/kafka/api/admin/CreateTopicRequest.scala PRE-CREATION 
  core/src/main/scala/kafka/api/admin/CreateTopicResponse.scala PRE-CREATION 
  core/src/main/scala/kafka/api/admin/DeleteTopicRequest.scala PRE-CREATION 
  core/src/main/scala/kafka/api/admin/DeleteTopicResponse.scala PRE-CREATION 
  core/src/main/scala/kafka/api/admin/DescribeTopicRequest.scala PRE-CREATION 
  core/src/main/scala/kafka/api/admin/DescribeTopicResponse.scala PRE-CREATION 
  core/src/main/scala/kafka/api/admin/ListTopicsRequest.scala PRE-CREATION 
  core/src/main/scala/kafka/api/admin/ListTopicsResponse.scala PRE-CREATION 
  core/src/main/scala/kafka/api/admin/PreferredReplicaLeaderElectionRequest.scala PRE-CREATION 
  core/src/main/scala/kafka/api/admin/PreferredReplicaLeaderElectionResponse.scala PRE-CREATION 
  core/src/main/scala/kafka/api/admin/ReassignPartitionsRequest.scala PRE-CREATION 
  core/src/main/scala/kafka/api/admin/ReassignPartitionsResponse.scala PRE-CREATION 
  core/src/main/scala/kafka/api/admin/VerifyPreferredReplicaLeaderElectionRequest.scala PRE-CREATION 
  core/src/main/scala/kafka/api/admin/VerifyPreferredReplicaLeaderElectionResponse.scala PRE-CREATION 
  core/src/main/scala/kafka/api/admin/VerifyReassignPartitionsRequest.scala PRE-CREATION 
  core/src/main/scala/kafka/api/admin/VerifyReassignPartitionsResponse.scala PRE-CREATION 
  core/src/main/scala/kafka/common/AdminRequestFailedException.scala PRE-CREATION 
  core/src/main/scala/kafka/common/ErrorMapping.scala eedc2f5f21dd8755fba891998456351622e17047 
  core/src/main/scala/kafka/common/InvalidRequestTargetException.scala PRE-CREATION 
  core/src/main/scala/kafka/controller/ControllerChannelManager.scala eb492f00449744bc8d63f55b393e2a1659d38454 
  core/src/main/scala/kafka/controller/KafkaController.scala 66df6d2fbdbdd556da6bea0df84f93e0472c8fbf 
  core/src/main/scala/kafka/server/KafkaApis.scala c011a1b79bd6c4e832fe7d097daacb0d647d1cd4 
  core/src/main/scala/kafka/server/MetadataCache.scala bf81a1ab88c14be8697b441eedbeb28fa0112643 
  core/src/main/scala/kafka/server/TopicCommandHelper.scala PRE-CREATION 
  core/src/main/scala/kafka/tools/ConsumerOffsetCheckerHelper.scala PRE-CREATION 
  core/src/main/scala/kafka/tools/PreferredReplicaLeaderElectionHelper.scala PRE-CREATION 
  core/src/main/scala/kafka/tools/ReassignPartitionsHelper.scala PRE-CREATION 
  core/src/test/scala/unit/kafka/api/RequestResponseSerializationTest.scala cd16ced5465d098be7a60498326b2a98c248f343 
  settings.gradle 83f764e6a4a15a5fdba232dce74a369870f26b45 
  tools/src/main/java/org/apache/kafka/cli/BaseCommandOpts.java PRE-CREATION 
  tools/src/main/java/org/apache/kafka/cli/Boot.java PRE-CREATION 
  tools/src/main/java/org/apache/kafka/cli/RequestDispatcher.java PRE-CREATION 
  tools/src/main/java/org/apache/kafka/cli/Shell.java PRE-CREATION 
  tools/src/main/java/org/apache/kafka/cli/command/AlterTopicCommand.java PRE-CREATION 
  tools/src/main/java/org/apache/kafka/cli/command/ClearScreenCommand.java PRE-CREATION 
  tools/src/main/java/org/apache/kafka/cli/command/Command.java PRE-CREATION 
  tools/src/main/java/org/apache/kafka/cli/command/CreateTopicCommand.java PRE-CREATION 
  tools/src/main/java/org/apache/kafka/cli/command/DeleteTopicCommand.java PRE-CREATION 
  tools/src/main/java/org/apache/kafka/cli/command/DescribeTopicCommand.java PRE-CREATION 
  tools/src/main/java/org/apache/kafka/cli/command/ExitCommand.java PRE-CREATION 
  tools/src/main/java/org/apache/kafka/cli/command/ListTopicsCommand.java PRE-CREATION 
  tools/src/main/java/org/apache/kafka/cli/command/PreferredReplicaLeaderElectionCommand.java PRE-CREATION 
  tools/src/main/java/org/apache/kafka/cli/command/PrintConsumerGroupOffsetsCommand.java PRE-CREATION 
  tools/src/main/java/org/apache/kafka/cli/command/PrintHelpCommand.java PRE-CREATION 
  tools/src/main/java/org/apache/kafka/cli/command/ReassignPartitionsCommand.java PRE-CREATION 
  tools/src/main/java/org/apache/kafka/cli/command/TopicSwitchCommand.java PRE-CREATION 
  tools/src/main/java/org/apache/kafka/cli/util/StringUtils.java PRE-CREATION 

Diff: https://reviews.apache.org/r/29301/diff/


Testing
-------


Thanks,

Andrii Biletskyi


Re: Review Request 29301: Patch for KAFKA-1694

Posted by Andrii Biletskyi <an...@stealth.ly>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29301/
-----------------------------------------------------------

(Updated Jan. 13, 2015, 5:30 p.m.)


Review request for kafka.


Bugs: KAFKA-1694
    https://issues.apache.org/jira/browse/KAFKA-1694


Repository: kafka


Description (updated)
-------

KAFKA-1694 - introduced new type for Wire protocol, ported ClusterMetadataResponse to it


KAFKA-1694 - Split Admin RQ/RP to separate messages


KAFKA-1694 - Admin commands can be handled only by controller; DeleteTopicCommand NPE fix


KAFKA-1776 - Ported ConsumerGroupOffsetChecker


KAFKA-1776 - Ported PreferredReplicaElectionTool and ReassignPartitionsTool to CLI


KAFKA-1694 - kafka-tools is uploaded on uploadAllArchives


KAFKA-1694 - ReviewBoard 29301 code review fixes


KAFKA-1694 - Data for ReassignPartitions and PreferredReplicaLeaderElection is in json string


KAFKA-1694 - Added logging


Diffs (updated)
-----

  bin/kafka.sh PRE-CREATION 
  bin/windows/kafka.bat PRE-CREATION 
  build.gradle ba52288031e2abc70e35e9297a4423dd5025950b 
  clients/src/main/java/org/apache/kafka/common/protocol/ApiKeys.java 109fc965e09b2ed186a073351bd037ac8af20a4c 
  clients/src/main/java/org/apache/kafka/common/protocol/Protocol.java 7517b879866fc5dad5f8d8ad30636da8bbe7784a 
  clients/src/main/java/org/apache/kafka/common/protocol/types/MaybeOf.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/protocol/types/Struct.java 121e880a941fcd3e6392859edba11a94236494cc 
  clients/src/main/java/org/apache/kafka/common/requests/ClusterMetadataRequest.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/requests/ClusterMetadataResponse.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/requests/admin/AlterTopicRequest.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/requests/admin/AlterTopicResponse.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/requests/admin/ConsumerGroupOffsetsRequest.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/requests/admin/ConsumerGroupOffsetsResponse.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/requests/admin/CreateTopicRequest.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/requests/admin/CreateTopicResponse.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/requests/admin/DeleteTopicRequest.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/requests/admin/DeleteTopicResponse.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/requests/admin/DescribeTopicOutput.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/requests/admin/DescribeTopicRequest.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/requests/admin/DescribeTopicResponse.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/requests/admin/ListTopicsOutput.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/requests/admin/ListTopicsRequest.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/requests/admin/ListTopicsResponse.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/requests/admin/PreferredReplicaLeaderElectionRequest.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/requests/admin/PreferredReplicaLeaderElectionResponse.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/requests/admin/ReassignPartitionsRequest.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/requests/admin/ReassignPartitionsResponse.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/requests/admin/TopicConfigDetails.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/requests/admin/TopicPartitionsDetails.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/requests/admin/VerifyPreferredReplicaLeaderElectionRequest.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/requests/admin/VerifyPreferredReplicaLeaderElectionResponse.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/requests/admin/VerifyReassignPartitionsRequest.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/requests/admin/VerifyReassignPartitionsResponse.java PRE-CREATION 
  clients/src/test/java/org/apache/kafka/common/requests/RequestResponseTest.java df37fc6d8f0db0b8192a948426af603be3444da4 
  config/tools-log4j.properties 52f07c96019b4083fc78f62cfb0a81080327e847 
  core/src/main/scala/kafka/api/ApiUtils.scala 1f80de1638978901500df808ca5133308c9d1fca 
  core/src/main/scala/kafka/api/ClusterMetadataRequest.scala PRE-CREATION 
  core/src/main/scala/kafka/api/ClusterMetadataResponse.scala PRE-CREATION 
  core/src/main/scala/kafka/api/RequestKeys.scala c24c0345feedc7b9e2e9f40af11bfa1b8d328c43 
  core/src/main/scala/kafka/api/admin/AlterTopicRequest.scala PRE-CREATION 
  core/src/main/scala/kafka/api/admin/AlterTopicResponse.scala PRE-CREATION 
  core/src/main/scala/kafka/api/admin/ConsumerGroupOffsetsRequest.scala PRE-CREATION 
  core/src/main/scala/kafka/api/admin/ConsumerGroupOffsetsResponse.scala PRE-CREATION 
  core/src/main/scala/kafka/api/admin/CreateTopicRequest.scala PRE-CREATION 
  core/src/main/scala/kafka/api/admin/CreateTopicResponse.scala PRE-CREATION 
  core/src/main/scala/kafka/api/admin/DeleteTopicRequest.scala PRE-CREATION 
  core/src/main/scala/kafka/api/admin/DeleteTopicResponse.scala PRE-CREATION 
  core/src/main/scala/kafka/api/admin/DescribeTopicRequest.scala PRE-CREATION 
  core/src/main/scala/kafka/api/admin/DescribeTopicResponse.scala PRE-CREATION 
  core/src/main/scala/kafka/api/admin/ListTopicsRequest.scala PRE-CREATION 
  core/src/main/scala/kafka/api/admin/ListTopicsResponse.scala PRE-CREATION 
  core/src/main/scala/kafka/api/admin/PreferredReplicaLeaderElectionRequest.scala PRE-CREATION 
  core/src/main/scala/kafka/api/admin/PreferredReplicaLeaderElectionResponse.scala PRE-CREATION 
  core/src/main/scala/kafka/api/admin/ReassignPartitionsRequest.scala PRE-CREATION 
  core/src/main/scala/kafka/api/admin/ReassignPartitionsResponse.scala PRE-CREATION 
  core/src/main/scala/kafka/api/admin/VerifyPreferredReplicaLeaderElectionRequest.scala PRE-CREATION 
  core/src/main/scala/kafka/api/admin/VerifyPreferredReplicaLeaderElectionResponse.scala PRE-CREATION 
  core/src/main/scala/kafka/api/admin/VerifyReassignPartitionsRequest.scala PRE-CREATION 
  core/src/main/scala/kafka/api/admin/VerifyReassignPartitionsResponse.scala PRE-CREATION 
  core/src/main/scala/kafka/common/AdminRequestFailedException.scala PRE-CREATION 
  core/src/main/scala/kafka/common/ErrorMapping.scala eedc2f5f21dd8755fba891998456351622e17047 
  core/src/main/scala/kafka/common/InvalidRequestTargetException.scala PRE-CREATION 
  core/src/main/scala/kafka/controller/ControllerChannelManager.scala eb492f00449744bc8d63f55b393e2a1659d38454 
  core/src/main/scala/kafka/controller/KafkaController.scala 66df6d2fbdbdd556da6bea0df84f93e0472c8fbf 
  core/src/main/scala/kafka/server/KafkaApis.scala c011a1b79bd6c4e832fe7d097daacb0d647d1cd4 
  core/src/main/scala/kafka/server/MetadataCache.scala bf81a1ab88c14be8697b441eedbeb28fa0112643 
  core/src/main/scala/kafka/server/TopicCommandHelper.scala PRE-CREATION 
  core/src/main/scala/kafka/tools/ConsumerOffsetCheckerHelper.scala PRE-CREATION 
  core/src/main/scala/kafka/tools/PreferredReplicaLeaderElectionHelper.scala PRE-CREATION 
  core/src/main/scala/kafka/tools/ReassignPartitionsHelper.scala PRE-CREATION 
  core/src/test/scala/unit/kafka/api/RequestResponseSerializationTest.scala cd16ced5465d098be7a60498326b2a98c248f343 
  settings.gradle 83f764e6a4a15a5fdba232dce74a369870f26b45 
  tools/src/main/java/org/apache/kafka/cli/BaseCommandOpts.java PRE-CREATION 
  tools/src/main/java/org/apache/kafka/cli/Boot.java PRE-CREATION 
  tools/src/main/java/org/apache/kafka/cli/RequestDispatcher.java PRE-CREATION 
  tools/src/main/java/org/apache/kafka/cli/Shell.java PRE-CREATION 
  tools/src/main/java/org/apache/kafka/cli/command/AlterTopicCommand.java PRE-CREATION 
  tools/src/main/java/org/apache/kafka/cli/command/ClearScreenCommand.java PRE-CREATION 
  tools/src/main/java/org/apache/kafka/cli/command/Command.java PRE-CREATION 
  tools/src/main/java/org/apache/kafka/cli/command/CreateTopicCommand.java PRE-CREATION 
  tools/src/main/java/org/apache/kafka/cli/command/DeleteTopicCommand.java PRE-CREATION 
  tools/src/main/java/org/apache/kafka/cli/command/DescribeTopicCommand.java PRE-CREATION 
  tools/src/main/java/org/apache/kafka/cli/command/ExitCommand.java PRE-CREATION 
  tools/src/main/java/org/apache/kafka/cli/command/ListTopicsCommand.java PRE-CREATION 
  tools/src/main/java/org/apache/kafka/cli/command/PreferredReplicaLeaderElectionCommand.java PRE-CREATION 
  tools/src/main/java/org/apache/kafka/cli/command/PrintConsumerGroupOffsetsCommand.java PRE-CREATION 
  tools/src/main/java/org/apache/kafka/cli/command/PrintHelpCommand.java PRE-CREATION 
  tools/src/main/java/org/apache/kafka/cli/command/ReassignPartitionsCommand.java PRE-CREATION 
  tools/src/main/java/org/apache/kafka/cli/command/TopicSwitchCommand.java PRE-CREATION 
  tools/src/main/java/org/apache/kafka/cli/util/StringUtils.java PRE-CREATION 

Diff: https://reviews.apache.org/r/29301/diff/


Testing
-------


Thanks,

Andrii Biletskyi


Re: Review Request 29301: Patch for KAFKA-1694

Posted by Andrii Biletskyi <an...@stealth.ly>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29301/
-----------------------------------------------------------

(Updated Jan. 12, 2015, 4:55 p.m.)


Review request for kafka.


Bugs: KAFKA-1694
    https://issues.apache.org/jira/browse/KAFKA-1694


Repository: kafka


Description (updated)
-------

KAFKA-1694 - introduced new type for Wire protocol, ported ClusterMetadataResponse to it


KAFKA-1694 - Split Admin RQ/RP to separate messages


KAFKA-1694 - Admin commands can be handled only by controller; DeleteTopicCommand NPE fix


KAFKA-1776 - Ported ConsumerGroupOffsetChecker


KAFKA-1776 - Ported PreferredReplicaElectionTool and ReassignPartitionsTool to CLI


KAFKA-1694 - kafka-tools is uploaded on uploadAllArchives


KAFKA-1694 - ReviewBoard 29301 code review fixes


Diffs (updated)
-----

  bin/kafka.sh PRE-CREATION 
  bin/windows/kafka.bat PRE-CREATION 
  build.gradle ba52288031e2abc70e35e9297a4423dd5025950b 
  clients/src/main/java/org/apache/kafka/common/protocol/ApiKeys.java 109fc965e09b2ed186a073351bd037ac8af20a4c 
  clients/src/main/java/org/apache/kafka/common/protocol/Protocol.java 7517b879866fc5dad5f8d8ad30636da8bbe7784a 
  clients/src/main/java/org/apache/kafka/common/protocol/types/MaybeOf.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/protocol/types/Struct.java 121e880a941fcd3e6392859edba11a94236494cc 
  clients/src/main/java/org/apache/kafka/common/requests/ClusterMetadataRequest.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/requests/ClusterMetadataResponse.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/requests/admin/AlterTopicRequest.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/requests/admin/AlterTopicResponse.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/requests/admin/ConsumerGroupOffsetsRequest.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/requests/admin/ConsumerGroupOffsetsResponse.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/requests/admin/CreateTopicRequest.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/requests/admin/CreateTopicResponse.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/requests/admin/DeleteTopicRequest.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/requests/admin/DeleteTopicResponse.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/requests/admin/DescribeTopicOutput.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/requests/admin/DescribeTopicRequest.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/requests/admin/DescribeTopicResponse.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/requests/admin/ListTopicsOutput.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/requests/admin/ListTopicsRequest.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/requests/admin/ListTopicsResponse.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/requests/admin/PreferredReplicaLeaderElectionRequest.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/requests/admin/PreferredReplicaLeaderElectionResponse.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/requests/admin/ReassignPartitionsRequest.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/requests/admin/ReassignPartitionsResponse.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/requests/admin/TopicConfigDetails.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/requests/admin/TopicPartitionsDetails.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/requests/admin/VerifyPreferredReplicaLeaderElectionRequest.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/requests/admin/VerifyPreferredReplicaLeaderElectionResponse.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/requests/admin/VerifyReassignPartitionsRequest.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/requests/admin/VerifyReassignPartitionsResponse.java PRE-CREATION 
  clients/src/test/java/org/apache/kafka/common/requests/RequestResponseTest.java df37fc6d8f0db0b8192a948426af603be3444da4 
  core/src/main/scala/kafka/api/ApiUtils.scala 1f80de1638978901500df808ca5133308c9d1fca 
  core/src/main/scala/kafka/api/ClusterMetadataRequest.scala PRE-CREATION 
  core/src/main/scala/kafka/api/ClusterMetadataResponse.scala PRE-CREATION 
  core/src/main/scala/kafka/api/RequestKeys.scala c24c0345feedc7b9e2e9f40af11bfa1b8d328c43 
  core/src/main/scala/kafka/api/admin/AlterTopicRequest.scala PRE-CREATION 
  core/src/main/scala/kafka/api/admin/AlterTopicResponse.scala PRE-CREATION 
  core/src/main/scala/kafka/api/admin/ConsumerGroupOffsetsRequest.scala PRE-CREATION 
  core/src/main/scala/kafka/api/admin/ConsumerGroupOffsetsResponse.scala PRE-CREATION 
  core/src/main/scala/kafka/api/admin/CreateTopicRequest.scala PRE-CREATION 
  core/src/main/scala/kafka/api/admin/CreateTopicResponse.scala PRE-CREATION 
  core/src/main/scala/kafka/api/admin/DeleteTopicRequest.scala PRE-CREATION 
  core/src/main/scala/kafka/api/admin/DeleteTopicResponse.scala PRE-CREATION 
  core/src/main/scala/kafka/api/admin/DescribeTopicRequest.scala PRE-CREATION 
  core/src/main/scala/kafka/api/admin/DescribeTopicResponse.scala PRE-CREATION 
  core/src/main/scala/kafka/api/admin/ListTopicsRequest.scala PRE-CREATION 
  core/src/main/scala/kafka/api/admin/ListTopicsResponse.scala PRE-CREATION 
  core/src/main/scala/kafka/api/admin/PreferredReplicaLeaderElectionRequest.scala PRE-CREATION 
  core/src/main/scala/kafka/api/admin/PreferredReplicaLeaderElectionResponse.scala PRE-CREATION 
  core/src/main/scala/kafka/api/admin/ReassignPartitionsRequest.scala PRE-CREATION 
  core/src/main/scala/kafka/api/admin/ReassignPartitionsResponse.scala PRE-CREATION 
  core/src/main/scala/kafka/api/admin/VerifyPreferredReplicaLeaderElectionRequest.scala PRE-CREATION 
  core/src/main/scala/kafka/api/admin/VerifyPreferredReplicaLeaderElectionResponse.scala PRE-CREATION 
  core/src/main/scala/kafka/api/admin/VerifyReassignPartitionsRequest.scala PRE-CREATION 
  core/src/main/scala/kafka/api/admin/VerifyReassignPartitionsResponse.scala PRE-CREATION 
  core/src/main/scala/kafka/common/AdminRequestFailedException.scala PRE-CREATION 
  core/src/main/scala/kafka/common/ErrorMapping.scala eedc2f5f21dd8755fba891998456351622e17047 
  core/src/main/scala/kafka/common/InvalidRequestTargetException.scala PRE-CREATION 
  core/src/main/scala/kafka/controller/ControllerChannelManager.scala eb492f00449744bc8d63f55b393e2a1659d38454 
  core/src/main/scala/kafka/controller/KafkaController.scala 66df6d2fbdbdd556da6bea0df84f93e0472c8fbf 
  core/src/main/scala/kafka/server/KafkaApis.scala c011a1b79bd6c4e832fe7d097daacb0d647d1cd4 
  core/src/main/scala/kafka/server/MetadataCache.scala bf81a1ab88c14be8697b441eedbeb28fa0112643 
  core/src/main/scala/kafka/server/TopicCommandHelper.scala PRE-CREATION 
  core/src/main/scala/kafka/tools/ConsumerOffsetCheckerHelper.scala PRE-CREATION 
  core/src/main/scala/kafka/tools/PreferredReplicaLeaderElectionHelper.scala PRE-CREATION 
  core/src/main/scala/kafka/tools/ReassignPartitionsHelper.scala PRE-CREATION 
  core/src/test/scala/unit/kafka/api/RequestResponseSerializationTest.scala cd16ced5465d098be7a60498326b2a98c248f343 
  settings.gradle 83f764e6a4a15a5fdba232dce74a369870f26b45 
  tools/src/main/java/org/apache/kafka/cli/BaseCommandOpts.java PRE-CREATION 
  tools/src/main/java/org/apache/kafka/cli/Boot.java PRE-CREATION 
  tools/src/main/java/org/apache/kafka/cli/RequestDispatcher.java PRE-CREATION 
  tools/src/main/java/org/apache/kafka/cli/Shell.java PRE-CREATION 
  tools/src/main/java/org/apache/kafka/cli/command/AlterTopicCommand.java PRE-CREATION 
  tools/src/main/java/org/apache/kafka/cli/command/ClearScreenCommand.java PRE-CREATION 
  tools/src/main/java/org/apache/kafka/cli/command/Command.java PRE-CREATION 
  tools/src/main/java/org/apache/kafka/cli/command/CreateTopicCommand.java PRE-CREATION 
  tools/src/main/java/org/apache/kafka/cli/command/DeleteTopicCommand.java PRE-CREATION 
  tools/src/main/java/org/apache/kafka/cli/command/DescribeTopicCommand.java PRE-CREATION 
  tools/src/main/java/org/apache/kafka/cli/command/ExitCommand.java PRE-CREATION 
  tools/src/main/java/org/apache/kafka/cli/command/ListTopicsCommand.java PRE-CREATION 
  tools/src/main/java/org/apache/kafka/cli/command/PreferredReplicaLeaderElectionCommand.java PRE-CREATION 
  tools/src/main/java/org/apache/kafka/cli/command/PrintConsumerGroupOffsetsCommand.java PRE-CREATION 
  tools/src/main/java/org/apache/kafka/cli/command/PrintHelpCommand.java PRE-CREATION 
  tools/src/main/java/org/apache/kafka/cli/command/ReassignPartitionsCommand.java PRE-CREATION 
  tools/src/main/java/org/apache/kafka/cli/command/TopicSwitchCommand.java PRE-CREATION 
  tools/src/main/java/org/apache/kafka/cli/util/StringUtils.java PRE-CREATION 

Diff: https://reviews.apache.org/r/29301/diff/


Testing
-------


Thanks,

Andrii Biletskyi


Re: Review Request 29301: Patch for KAFKA-1694

Posted by Andrii Biletskyi <an...@stealth.ly>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29301/
-----------------------------------------------------------

(Updated Jan. 12, 2015, 1:28 p.m.)


Review request for kafka.


Bugs: KAFKA-1694
    https://issues.apache.org/jira/browse/KAFKA-1694


Repository: kafka


Description (updated)
-------

KAFKA-1694 - introduced new type for Wire protocol, ported ClusterMetadataResponse to it


KAFKA-1694 - Split Admin RQ/RP to separate messages


KAFKA-1694 - Admin commands can be handled only by controller; DeleteTopicCommand NPE fix


KAFKA-1776 - Ported ConsumerGroupOffsetChecker


KAFKA-1776 - Ported PreferredReplicaElectionTool and ReassignPartitionsTool to CLI


Diffs (updated)
-----

  bin/kafka.sh PRE-CREATION 
  bin/windows/kafka.bat PRE-CREATION 
  build.gradle ba52288031e2abc70e35e9297a4423dd5025950b 
  clients/src/main/java/org/apache/kafka/common/protocol/ApiKeys.java 109fc965e09b2ed186a073351bd037ac8af20a4c 
  clients/src/main/java/org/apache/kafka/common/protocol/Protocol.java 7517b879866fc5dad5f8d8ad30636da8bbe7784a 
  clients/src/main/java/org/apache/kafka/common/protocol/types/MaybeOf.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/protocol/types/Struct.java 121e880a941fcd3e6392859edba11a94236494cc 
  clients/src/main/java/org/apache/kafka/common/requests/ClusterMetadataRequest.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/requests/ClusterMetadataResponse.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/requests/admin/AlterTopicRequest.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/requests/admin/AlterTopicResponse.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/requests/admin/ConsumerGroupOffsetsRequest.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/requests/admin/ConsumerGroupOffsetsResponse.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/requests/admin/CreateTopicRequest.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/requests/admin/CreateTopicResponse.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/requests/admin/DeleteTopicRequest.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/requests/admin/DeleteTopicResponse.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/requests/admin/DescribeTopicOutput.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/requests/admin/DescribeTopicRequest.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/requests/admin/DescribeTopicResponse.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/requests/admin/ListTopicsOutput.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/requests/admin/ListTopicsRequest.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/requests/admin/ListTopicsResponse.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/requests/admin/PreferredReplicaLeaderElectionRequest.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/requests/admin/PreferredReplicaLeaderElectionResponse.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/requests/admin/ReassignPartitionsRequest.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/requests/admin/ReassignPartitionsResponse.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/requests/admin/TopicConfigDetails.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/requests/admin/TopicPartitionsDetails.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/requests/admin/VerifyPreferredReplicaLeaderElectionRequest.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/requests/admin/VerifyPreferredReplicaLeaderElectionResponse.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/requests/admin/VerifyReassignPartitionsRequest.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/requests/admin/VerifyReassignPartitionsResponse.java PRE-CREATION 
  clients/src/test/java/org/apache/kafka/common/requests/RequestResponseTest.java df37fc6d8f0db0b8192a948426af603be3444da4 
  core/src/main/scala/kafka/api/ApiUtils.scala 1f80de1638978901500df808ca5133308c9d1fca 
  core/src/main/scala/kafka/api/ClusterMetadataRequest.scala PRE-CREATION 
  core/src/main/scala/kafka/api/ClusterMetadataResponse.scala PRE-CREATION 
  core/src/main/scala/kafka/api/RequestKeys.scala c24c0345feedc7b9e2e9f40af11bfa1b8d328c43 
  core/src/main/scala/kafka/api/admin/AlterTopicRequest.scala PRE-CREATION 
  core/src/main/scala/kafka/api/admin/AlterTopicResponse.scala PRE-CREATION 
  core/src/main/scala/kafka/api/admin/ConsumerGroupOffsetsRequest.scala PRE-CREATION 
  core/src/main/scala/kafka/api/admin/ConsumerGroupOffsetsResponse.scala PRE-CREATION 
  core/src/main/scala/kafka/api/admin/CreateTopicRequest.scala PRE-CREATION 
  core/src/main/scala/kafka/api/admin/CreateTopicResponse.scala PRE-CREATION 
  core/src/main/scala/kafka/api/admin/DeleteTopicRequest.scala PRE-CREATION 
  core/src/main/scala/kafka/api/admin/DeleteTopicResponse.scala PRE-CREATION 
  core/src/main/scala/kafka/api/admin/DescribeTopicRequest.scala PRE-CREATION 
  core/src/main/scala/kafka/api/admin/DescribeTopicResponse.scala PRE-CREATION 
  core/src/main/scala/kafka/api/admin/ListTopicsRequest.scala PRE-CREATION 
  core/src/main/scala/kafka/api/admin/ListTopicsResponse.scala PRE-CREATION 
  core/src/main/scala/kafka/api/admin/PreferredReplicaLeaderElectionRequest.scala PRE-CREATION 
  core/src/main/scala/kafka/api/admin/PreferredReplicaLeaderElectionResponse.scala PRE-CREATION 
  core/src/main/scala/kafka/api/admin/ReassignPartitionsRequest.scala PRE-CREATION 
  core/src/main/scala/kafka/api/admin/ReassignPartitionsResponse.scala PRE-CREATION 
  core/src/main/scala/kafka/api/admin/VerifyPreferredReplicaLeaderElectionRequest.scala PRE-CREATION 
  core/src/main/scala/kafka/api/admin/VerifyPreferredReplicaLeaderElectionResponse.scala PRE-CREATION 
  core/src/main/scala/kafka/api/admin/VerifyReassignPartitionsRequest.scala PRE-CREATION 
  core/src/main/scala/kafka/api/admin/VerifyReassignPartitionsResponse.scala PRE-CREATION 
  core/src/main/scala/kafka/common/AdminRequestFailedException.scala PRE-CREATION 
  core/src/main/scala/kafka/common/ErrorMapping.scala eedc2f5f21dd8755fba891998456351622e17047 
  core/src/main/scala/kafka/common/InvalidRequestTargetException.scala PRE-CREATION 
  core/src/main/scala/kafka/controller/ControllerChannelManager.scala eb492f00449744bc8d63f55b393e2a1659d38454 
  core/src/main/scala/kafka/controller/KafkaController.scala 66df6d2fbdbdd556da6bea0df84f93e0472c8fbf 
  core/src/main/scala/kafka/server/KafkaApis.scala c011a1b79bd6c4e832fe7d097daacb0d647d1cd4 
  core/src/main/scala/kafka/server/MetadataCache.scala bf81a1ab88c14be8697b441eedbeb28fa0112643 
  core/src/main/scala/kafka/server/TopicCommandHelper.scala PRE-CREATION 
  core/src/main/scala/kafka/tools/ConsumerOffsetCheckerHelper.scala PRE-CREATION 
  core/src/main/scala/kafka/tools/PreferredReplicaLeaderElectionHelper.scala PRE-CREATION 
  core/src/main/scala/kafka/tools/ReassignPartitionsHelper.scala PRE-CREATION 
  core/src/test/scala/unit/kafka/api/RequestResponseSerializationTest.scala cd16ced5465d098be7a60498326b2a98c248f343 
  settings.gradle 83f764e6a4a15a5fdba232dce74a369870f26b45 
  tools/src/main/java/org/apache/kafka/cli/BaseCommandOpts.java PRE-CREATION 
  tools/src/main/java/org/apache/kafka/cli/Boot.java PRE-CREATION 
  tools/src/main/java/org/apache/kafka/cli/RequestDispatcher.java PRE-CREATION 
  tools/src/main/java/org/apache/kafka/cli/Shell.java PRE-CREATION 
  tools/src/main/java/org/apache/kafka/cli/command/AlterTopicCommand.java PRE-CREATION 
  tools/src/main/java/org/apache/kafka/cli/command/ClearScreenCommand.java PRE-CREATION 
  tools/src/main/java/org/apache/kafka/cli/command/Command.java PRE-CREATION 
  tools/src/main/java/org/apache/kafka/cli/command/CreateTopicCommand.java PRE-CREATION 
  tools/src/main/java/org/apache/kafka/cli/command/DeleteTopicCommand.java PRE-CREATION 
  tools/src/main/java/org/apache/kafka/cli/command/DescribeTopicCommand.java PRE-CREATION 
  tools/src/main/java/org/apache/kafka/cli/command/ExitCommand.java PRE-CREATION 
  tools/src/main/java/org/apache/kafka/cli/command/ListTopicsCommand.java PRE-CREATION 
  tools/src/main/java/org/apache/kafka/cli/command/PreferredReplicaLeaderElectionCommand.java PRE-CREATION 
  tools/src/main/java/org/apache/kafka/cli/command/PrintConsumerGroupOffsetsCommand.java PRE-CREATION 
  tools/src/main/java/org/apache/kafka/cli/command/PrintHelpCommand.java PRE-CREATION 
  tools/src/main/java/org/apache/kafka/cli/command/ReassignPartitionsCommand.java PRE-CREATION 
  tools/src/main/java/org/apache/kafka/cli/command/TopicSwitchCommand.java PRE-CREATION 
  tools/src/main/java/org/apache/kafka/cli/util/StringUtils.java PRE-CREATION 

Diff: https://reviews.apache.org/r/29301/diff/


Testing
-------


Thanks,

Andrii Biletskyi


Re: Review Request 29301: Patch for KAFKA-1694

Posted by Andrii Biletskyi <an...@stealth.ly>.

> On Jan. 6, 2015, 8:19 a.m., Joe Stein wrote:
> > build.gradle, line 211
> > <https://reviews.apache.org/r/29301/diff/2/?file=800278#file800278line211>
> >
> >     If we can do this without an upgrade that would be great if we are in fact just requiring 1 function.

This feature allows us to call CLI commands both from interactive shell (kafka>) and right from ./kafka.sh (e.g. ./kafka.sh--create-topic).
The difference is that for ./kafka.sh we always have to supply one additional option --controller which is not recognized during command execution from interactive Shell.
As a workaround I can process --controller separately and then simply "cut off" it from command line args array but this looks a bit lame.


> On Jan. 6, 2015, 8:19 a.m., Joe Stein wrote:
> > clients/src/main/java/org/apache/kafka/common/protocol/types/MaybeOf.java, line 21
> > <https://reviews.apache.org/r/29301/diff/2/?file=800281#file800281line21>
> >
> >     not sure about this name, you are making an Option[] for the protocol value and that not make sense if you look at the use of it and not how it works.

As discussed for now better naming option wasn't found:). I'm happy to change if someone comes up with a good one.
I also added some comments to the class to make it more explanatory.


> On Jan. 6, 2015, 8:19 a.m., Joe Stein wrote:
> > clients/src/main/java/org/apache/kafka/common/protocol/types/MaybeOf.java, line 51
> > <https://reviews.apache.org/r/29301/diff/2/?file=800281#file800281line51>
> >
> >     why wouldn't the null size == 0?

There is always an additional byte showing whether value is present.


- Andrii


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29301/#review66801
-----------------------------------------------------------


On Jan. 12, 2015, 4:55 p.m., Andrii Biletskyi wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29301/
> -----------------------------------------------------------
> 
> (Updated Jan. 12, 2015, 4:55 p.m.)
> 
> 
> Review request for kafka.
> 
> 
> Bugs: KAFKA-1694
>     https://issues.apache.org/jira/browse/KAFKA-1694
> 
> 
> Repository: kafka
> 
> 
> Description
> -------
> 
> KAFKA-1694 - introduced new type for Wire protocol, ported ClusterMetadataResponse to it
> 
> 
> KAFKA-1694 - Split Admin RQ/RP to separate messages
> 
> 
> KAFKA-1694 - Admin commands can be handled only by controller; DeleteTopicCommand NPE fix
> 
> 
> KAFKA-1776 - Ported ConsumerGroupOffsetChecker
> 
> 
> KAFKA-1776 - Ported PreferredReplicaElectionTool and ReassignPartitionsTool to CLI
> 
> 
> KAFKA-1694 - kafka-tools is uploaded on uploadAllArchives
> 
> 
> KAFKA-1694 - ReviewBoard 29301 code review fixes
> 
> 
> Diffs
> -----
> 
>   bin/kafka.sh PRE-CREATION 
>   bin/windows/kafka.bat PRE-CREATION 
>   build.gradle ba52288031e2abc70e35e9297a4423dd5025950b 
>   clients/src/main/java/org/apache/kafka/common/protocol/ApiKeys.java 109fc965e09b2ed186a073351bd037ac8af20a4c 
>   clients/src/main/java/org/apache/kafka/common/protocol/Protocol.java 7517b879866fc5dad5f8d8ad30636da8bbe7784a 
>   clients/src/main/java/org/apache/kafka/common/protocol/types/MaybeOf.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/protocol/types/Struct.java 121e880a941fcd3e6392859edba11a94236494cc 
>   clients/src/main/java/org/apache/kafka/common/requests/ClusterMetadataRequest.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/ClusterMetadataResponse.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/AlterTopicRequest.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/AlterTopicResponse.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/ConsumerGroupOffsetsRequest.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/ConsumerGroupOffsetsResponse.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/CreateTopicRequest.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/CreateTopicResponse.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/DeleteTopicRequest.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/DeleteTopicResponse.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/DescribeTopicOutput.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/DescribeTopicRequest.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/DescribeTopicResponse.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/ListTopicsOutput.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/ListTopicsRequest.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/ListTopicsResponse.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/PreferredReplicaLeaderElectionRequest.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/PreferredReplicaLeaderElectionResponse.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/ReassignPartitionsRequest.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/ReassignPartitionsResponse.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/TopicConfigDetails.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/TopicPartitionsDetails.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/VerifyPreferredReplicaLeaderElectionRequest.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/VerifyPreferredReplicaLeaderElectionResponse.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/VerifyReassignPartitionsRequest.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/VerifyReassignPartitionsResponse.java PRE-CREATION 
>   clients/src/test/java/org/apache/kafka/common/requests/RequestResponseTest.java df37fc6d8f0db0b8192a948426af603be3444da4 
>   core/src/main/scala/kafka/api/ApiUtils.scala 1f80de1638978901500df808ca5133308c9d1fca 
>   core/src/main/scala/kafka/api/ClusterMetadataRequest.scala PRE-CREATION 
>   core/src/main/scala/kafka/api/ClusterMetadataResponse.scala PRE-CREATION 
>   core/src/main/scala/kafka/api/RequestKeys.scala c24c0345feedc7b9e2e9f40af11bfa1b8d328c43 
>   core/src/main/scala/kafka/api/admin/AlterTopicRequest.scala PRE-CREATION 
>   core/src/main/scala/kafka/api/admin/AlterTopicResponse.scala PRE-CREATION 
>   core/src/main/scala/kafka/api/admin/ConsumerGroupOffsetsRequest.scala PRE-CREATION 
>   core/src/main/scala/kafka/api/admin/ConsumerGroupOffsetsResponse.scala PRE-CREATION 
>   core/src/main/scala/kafka/api/admin/CreateTopicRequest.scala PRE-CREATION 
>   core/src/main/scala/kafka/api/admin/CreateTopicResponse.scala PRE-CREATION 
>   core/src/main/scala/kafka/api/admin/DeleteTopicRequest.scala PRE-CREATION 
>   core/src/main/scala/kafka/api/admin/DeleteTopicResponse.scala PRE-CREATION 
>   core/src/main/scala/kafka/api/admin/DescribeTopicRequest.scala PRE-CREATION 
>   core/src/main/scala/kafka/api/admin/DescribeTopicResponse.scala PRE-CREATION 
>   core/src/main/scala/kafka/api/admin/ListTopicsRequest.scala PRE-CREATION 
>   core/src/main/scala/kafka/api/admin/ListTopicsResponse.scala PRE-CREATION 
>   core/src/main/scala/kafka/api/admin/PreferredReplicaLeaderElectionRequest.scala PRE-CREATION 
>   core/src/main/scala/kafka/api/admin/PreferredReplicaLeaderElectionResponse.scala PRE-CREATION 
>   core/src/main/scala/kafka/api/admin/ReassignPartitionsRequest.scala PRE-CREATION 
>   core/src/main/scala/kafka/api/admin/ReassignPartitionsResponse.scala PRE-CREATION 
>   core/src/main/scala/kafka/api/admin/VerifyPreferredReplicaLeaderElectionRequest.scala PRE-CREATION 
>   core/src/main/scala/kafka/api/admin/VerifyPreferredReplicaLeaderElectionResponse.scala PRE-CREATION 
>   core/src/main/scala/kafka/api/admin/VerifyReassignPartitionsRequest.scala PRE-CREATION 
>   core/src/main/scala/kafka/api/admin/VerifyReassignPartitionsResponse.scala PRE-CREATION 
>   core/src/main/scala/kafka/common/AdminRequestFailedException.scala PRE-CREATION 
>   core/src/main/scala/kafka/common/ErrorMapping.scala eedc2f5f21dd8755fba891998456351622e17047 
>   core/src/main/scala/kafka/common/InvalidRequestTargetException.scala PRE-CREATION 
>   core/src/main/scala/kafka/controller/ControllerChannelManager.scala eb492f00449744bc8d63f55b393e2a1659d38454 
>   core/src/main/scala/kafka/controller/KafkaController.scala 66df6d2fbdbdd556da6bea0df84f93e0472c8fbf 
>   core/src/main/scala/kafka/server/KafkaApis.scala c011a1b79bd6c4e832fe7d097daacb0d647d1cd4 
>   core/src/main/scala/kafka/server/MetadataCache.scala bf81a1ab88c14be8697b441eedbeb28fa0112643 
>   core/src/main/scala/kafka/server/TopicCommandHelper.scala PRE-CREATION 
>   core/src/main/scala/kafka/tools/ConsumerOffsetCheckerHelper.scala PRE-CREATION 
>   core/src/main/scala/kafka/tools/PreferredReplicaLeaderElectionHelper.scala PRE-CREATION 
>   core/src/main/scala/kafka/tools/ReassignPartitionsHelper.scala PRE-CREATION 
>   core/src/test/scala/unit/kafka/api/RequestResponseSerializationTest.scala cd16ced5465d098be7a60498326b2a98c248f343 
>   settings.gradle 83f764e6a4a15a5fdba232dce74a369870f26b45 
>   tools/src/main/java/org/apache/kafka/cli/BaseCommandOpts.java PRE-CREATION 
>   tools/src/main/java/org/apache/kafka/cli/Boot.java PRE-CREATION 
>   tools/src/main/java/org/apache/kafka/cli/RequestDispatcher.java PRE-CREATION 
>   tools/src/main/java/org/apache/kafka/cli/Shell.java PRE-CREATION 
>   tools/src/main/java/org/apache/kafka/cli/command/AlterTopicCommand.java PRE-CREATION 
>   tools/src/main/java/org/apache/kafka/cli/command/ClearScreenCommand.java PRE-CREATION 
>   tools/src/main/java/org/apache/kafka/cli/command/Command.java PRE-CREATION 
>   tools/src/main/java/org/apache/kafka/cli/command/CreateTopicCommand.java PRE-CREATION 
>   tools/src/main/java/org/apache/kafka/cli/command/DeleteTopicCommand.java PRE-CREATION 
>   tools/src/main/java/org/apache/kafka/cli/command/DescribeTopicCommand.java PRE-CREATION 
>   tools/src/main/java/org/apache/kafka/cli/command/ExitCommand.java PRE-CREATION 
>   tools/src/main/java/org/apache/kafka/cli/command/ListTopicsCommand.java PRE-CREATION 
>   tools/src/main/java/org/apache/kafka/cli/command/PreferredReplicaLeaderElectionCommand.java PRE-CREATION 
>   tools/src/main/java/org/apache/kafka/cli/command/PrintConsumerGroupOffsetsCommand.java PRE-CREATION 
>   tools/src/main/java/org/apache/kafka/cli/command/PrintHelpCommand.java PRE-CREATION 
>   tools/src/main/java/org/apache/kafka/cli/command/ReassignPartitionsCommand.java PRE-CREATION 
>   tools/src/main/java/org/apache/kafka/cli/command/TopicSwitchCommand.java PRE-CREATION 
>   tools/src/main/java/org/apache/kafka/cli/util/StringUtils.java PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/29301/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Andrii Biletskyi
> 
>


Re: Review Request 29301: Patch for KAFKA-1694

Posted by Joe Stein <cr...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29301/#review66801
-----------------------------------------------------------



build.gradle
<https://reviews.apache.org/r/29301/#comment110428>

    We need to have upload archive here



build.gradle
<https://reviews.apache.org/r/29301/#comment110431>

    If we can do this without an upgrade that would be great if we are in fact just requiring 1 function.



build.gradle
<https://reviews.apache.org/r/29301/#comment110432>

    ???



clients/src/main/java/org/apache/kafka/common/protocol/types/MaybeOf.java
<https://reviews.apache.org/r/29301/#comment110437>

    not sure about this name, you are making an Option[] for the protocol value and that not make sense if you look at the use of it and not how it works.



clients/src/main/java/org/apache/kafka/common/protocol/types/MaybeOf.java
<https://reviews.apache.org/r/29301/#comment110439>

    why wouldn't the null size == 0?



core/src/main/scala/kafka/common/ErrorMapping.scala
<https://reviews.apache.org/r/29301/#comment110440>

    This is for trying to send an admin request to a broker that is not the controller? I think it should be name for that more specifically and clearly/


- Joe Stein


On Dec. 24, 2014, 7:22 p.m., Andrii Biletskyi wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29301/
> -----------------------------------------------------------
> 
> (Updated Dec. 24, 2014, 7:22 p.m.)
> 
> 
> Review request for kafka.
> 
> 
> Bugs: KAFKA-1694
>     https://issues.apache.org/jira/browse/KAFKA-1694
> 
> 
> Repository: kafka
> 
> 
> Description
> -------
> 
> KAFKA-1694 - introduced new type for Wire protocol, ported ClusterMetadataResponse to it
> 
> 
> KAFKA-1694 - Split Admin RQ/RP to separate messages
> 
> 
> KAFKA-1694 - Admin commands can be handled only by controller; DeleteTopicCommand NPE fix
> 
> 
> Diffs
> -----
> 
>   bin/kafka.sh PRE-CREATION 
>   bin/windows/kafka.bat PRE-CREATION 
>   build.gradle 18f86e4c8a10618d50ac78572d119c6e100ed85b 
>   clients/src/main/java/org/apache/kafka/common/protocol/ApiKeys.java 109fc965e09b2ed186a073351bd037ac8af20a4c 
>   clients/src/main/java/org/apache/kafka/common/protocol/Protocol.java 7517b879866fc5dad5f8d8ad30636da8bbe7784a 
>   clients/src/main/java/org/apache/kafka/common/protocol/types/MaybeOf.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/protocol/types/Struct.java 121e880a941fcd3e6392859edba11a94236494cc 
>   clients/src/main/java/org/apache/kafka/common/requests/ClusterMetadataRequest.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/ClusterMetadataResponse.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/AlterTopicRequest.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/AlterTopicResponse.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/CreateTopicRequest.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/CreateTopicResponse.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/DeleteTopicRequest.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/DeleteTopicResponse.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/DescribeTopicOutput.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/DescribeTopicRequest.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/DescribeTopicResponse.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/ListTopicsOutput.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/ListTopicsRequest.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/ListTopicsResponse.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/TopicConfigDetails.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/requests/admin/TopicPartitionsDetails.java PRE-CREATION 
>   clients/src/test/java/org/apache/kafka/common/requests/RequestResponseTest.java df37fc6d8f0db0b8192a948426af603be3444da4 
>   core/src/main/scala/kafka/api/ApiUtils.scala 1f80de1638978901500df808ca5133308c9d1fca 
>   core/src/main/scala/kafka/api/ClusterMetadataRequest.scala PRE-CREATION 
>   core/src/main/scala/kafka/api/ClusterMetadataResponse.scala PRE-CREATION 
>   core/src/main/scala/kafka/api/RequestKeys.scala c24c0345feedc7b9e2e9f40af11bfa1b8d328c43 
>   core/src/main/scala/kafka/api/admin/AlterTopicRequest.scala PRE-CREATION 
>   core/src/main/scala/kafka/api/admin/AlterTopicResponse.scala PRE-CREATION 
>   core/src/main/scala/kafka/api/admin/CreateTopicRequest.scala PRE-CREATION 
>   core/src/main/scala/kafka/api/admin/CreateTopicResponse.scala PRE-CREATION 
>   core/src/main/scala/kafka/api/admin/DeleteTopicRequest.scala PRE-CREATION 
>   core/src/main/scala/kafka/api/admin/DeleteTopicResponse.scala PRE-CREATION 
>   core/src/main/scala/kafka/api/admin/DescribeTopicRequest.scala PRE-CREATION 
>   core/src/main/scala/kafka/api/admin/DescribeTopicResponse.scala PRE-CREATION 
>   core/src/main/scala/kafka/api/admin/ListTopicsRequest.scala PRE-CREATION 
>   core/src/main/scala/kafka/api/admin/ListTopicsResponse.scala PRE-CREATION 
>   core/src/main/scala/kafka/common/AdminRequestFailedException.scala PRE-CREATION 
>   core/src/main/scala/kafka/common/ErrorMapping.scala eedc2f5f21dd8755fba891998456351622e17047 
>   core/src/main/scala/kafka/common/InvalidRequestTargetException.scala PRE-CREATION 
>   core/src/main/scala/kafka/controller/ControllerChannelManager.scala eb492f00449744bc8d63f55b393e2a1659d38454 
>   core/src/main/scala/kafka/controller/KafkaController.scala 66df6d2fbdbdd556da6bea0df84f93e0472c8fbf 
>   core/src/main/scala/kafka/server/KafkaApis.scala 2a1c0326b6e6966d8b8254bd6a1cb83ad98a3b80 
>   core/src/main/scala/kafka/server/MetadataCache.scala bf81a1ab88c14be8697b441eedbeb28fa0112643 
>   core/src/main/scala/kafka/server/TopicCommandHelper.scala PRE-CREATION 
>   core/src/test/scala/unit/kafka/api/RequestResponseSerializationTest.scala cd16ced5465d098be7a60498326b2a98c248f343 
>   settings.gradle 83f764e6a4a15a5fdba232dce74a369870f26b45 
>   tools/src/main/java/org/apache/kafka/cli/BaseCommandOpts.java PRE-CREATION 
>   tools/src/main/java/org/apache/kafka/cli/Boot.java PRE-CREATION 
>   tools/src/main/java/org/apache/kafka/cli/RequestDispatcher.java PRE-CREATION 
>   tools/src/main/java/org/apache/kafka/cli/Shell.java PRE-CREATION 
>   tools/src/main/java/org/apache/kafka/cli/command/AlterTopicCommand.java PRE-CREATION 
>   tools/src/main/java/org/apache/kafka/cli/command/ClearScreenCommand.java PRE-CREATION 
>   tools/src/main/java/org/apache/kafka/cli/command/Command.java PRE-CREATION 
>   tools/src/main/java/org/apache/kafka/cli/command/CreateTopicCommand.java PRE-CREATION 
>   tools/src/main/java/org/apache/kafka/cli/command/DeleteTopicCommand.java PRE-CREATION 
>   tools/src/main/java/org/apache/kafka/cli/command/DescribeTopicCommand.java PRE-CREATION 
>   tools/src/main/java/org/apache/kafka/cli/command/ExitCommand.java PRE-CREATION 
>   tools/src/main/java/org/apache/kafka/cli/command/ListTopicsCommand.java PRE-CREATION 
>   tools/src/main/java/org/apache/kafka/cli/command/PrintHelpCommand.java PRE-CREATION 
>   tools/src/main/java/org/apache/kafka/cli/command/TopicSwitchCommand.java PRE-CREATION 
>   tools/src/main/java/org/apache/kafka/cli/util/StringUtils.java PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/29301/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Andrii Biletskyi
> 
>


Re: Review Request 29301: Patch for KAFKA-1694

Posted by Andrii Biletskyi <an...@stealth.ly>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29301/
-----------------------------------------------------------

(Updated Dec. 24, 2014, 7:22 p.m.)


Review request for kafka.


Bugs: KAFKA-1694
    https://issues.apache.org/jira/browse/KAFKA-1694


Repository: kafka


Description (updated)
-------

KAFKA-1694 - introduced new type for Wire protocol, ported ClusterMetadataResponse to it


KAFKA-1694 - Split Admin RQ/RP to separate messages


KAFKA-1694 - Admin commands can be handled only by controller; DeleteTopicCommand NPE fix


Diffs (updated)
-----

  bin/kafka.sh PRE-CREATION 
  bin/windows/kafka.bat PRE-CREATION 
  build.gradle 18f86e4c8a10618d50ac78572d119c6e100ed85b 
  clients/src/main/java/org/apache/kafka/common/protocol/ApiKeys.java 109fc965e09b2ed186a073351bd037ac8af20a4c 
  clients/src/main/java/org/apache/kafka/common/protocol/Protocol.java 7517b879866fc5dad5f8d8ad30636da8bbe7784a 
  clients/src/main/java/org/apache/kafka/common/protocol/types/MaybeOf.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/protocol/types/Struct.java 121e880a941fcd3e6392859edba11a94236494cc 
  clients/src/main/java/org/apache/kafka/common/requests/ClusterMetadataRequest.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/requests/ClusterMetadataResponse.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/requests/admin/AlterTopicRequest.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/requests/admin/AlterTopicResponse.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/requests/admin/CreateTopicRequest.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/requests/admin/CreateTopicResponse.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/requests/admin/DeleteTopicRequest.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/requests/admin/DeleteTopicResponse.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/requests/admin/DescribeTopicOutput.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/requests/admin/DescribeTopicRequest.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/requests/admin/DescribeTopicResponse.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/requests/admin/ListTopicsOutput.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/requests/admin/ListTopicsRequest.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/requests/admin/ListTopicsResponse.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/requests/admin/TopicConfigDetails.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/requests/admin/TopicPartitionsDetails.java PRE-CREATION 
  clients/src/test/java/org/apache/kafka/common/requests/RequestResponseTest.java df37fc6d8f0db0b8192a948426af603be3444da4 
  core/src/main/scala/kafka/api/ApiUtils.scala 1f80de1638978901500df808ca5133308c9d1fca 
  core/src/main/scala/kafka/api/ClusterMetadataRequest.scala PRE-CREATION 
  core/src/main/scala/kafka/api/ClusterMetadataResponse.scala PRE-CREATION 
  core/src/main/scala/kafka/api/RequestKeys.scala c24c0345feedc7b9e2e9f40af11bfa1b8d328c43 
  core/src/main/scala/kafka/api/admin/AlterTopicRequest.scala PRE-CREATION 
  core/src/main/scala/kafka/api/admin/AlterTopicResponse.scala PRE-CREATION 
  core/src/main/scala/kafka/api/admin/CreateTopicRequest.scala PRE-CREATION 
  core/src/main/scala/kafka/api/admin/CreateTopicResponse.scala PRE-CREATION 
  core/src/main/scala/kafka/api/admin/DeleteTopicRequest.scala PRE-CREATION 
  core/src/main/scala/kafka/api/admin/DeleteTopicResponse.scala PRE-CREATION 
  core/src/main/scala/kafka/api/admin/DescribeTopicRequest.scala PRE-CREATION 
  core/src/main/scala/kafka/api/admin/DescribeTopicResponse.scala PRE-CREATION 
  core/src/main/scala/kafka/api/admin/ListTopicsRequest.scala PRE-CREATION 
  core/src/main/scala/kafka/api/admin/ListTopicsResponse.scala PRE-CREATION 
  core/src/main/scala/kafka/common/AdminRequestFailedException.scala PRE-CREATION 
  core/src/main/scala/kafka/common/ErrorMapping.scala eedc2f5f21dd8755fba891998456351622e17047 
  core/src/main/scala/kafka/common/InvalidRequestTargetException.scala PRE-CREATION 
  core/src/main/scala/kafka/controller/ControllerChannelManager.scala eb492f00449744bc8d63f55b393e2a1659d38454 
  core/src/main/scala/kafka/controller/KafkaController.scala 66df6d2fbdbdd556da6bea0df84f93e0472c8fbf 
  core/src/main/scala/kafka/server/KafkaApis.scala 2a1c0326b6e6966d8b8254bd6a1cb83ad98a3b80 
  core/src/main/scala/kafka/server/MetadataCache.scala bf81a1ab88c14be8697b441eedbeb28fa0112643 
  core/src/main/scala/kafka/server/TopicCommandHelper.scala PRE-CREATION 
  core/src/test/scala/unit/kafka/api/RequestResponseSerializationTest.scala cd16ced5465d098be7a60498326b2a98c248f343 
  settings.gradle 83f764e6a4a15a5fdba232dce74a369870f26b45 
  tools/src/main/java/org/apache/kafka/cli/BaseCommandOpts.java PRE-CREATION 
  tools/src/main/java/org/apache/kafka/cli/Boot.java PRE-CREATION 
  tools/src/main/java/org/apache/kafka/cli/RequestDispatcher.java PRE-CREATION 
  tools/src/main/java/org/apache/kafka/cli/Shell.java PRE-CREATION 
  tools/src/main/java/org/apache/kafka/cli/command/AlterTopicCommand.java PRE-CREATION 
  tools/src/main/java/org/apache/kafka/cli/command/ClearScreenCommand.java PRE-CREATION 
  tools/src/main/java/org/apache/kafka/cli/command/Command.java PRE-CREATION 
  tools/src/main/java/org/apache/kafka/cli/command/CreateTopicCommand.java PRE-CREATION 
  tools/src/main/java/org/apache/kafka/cli/command/DeleteTopicCommand.java PRE-CREATION 
  tools/src/main/java/org/apache/kafka/cli/command/DescribeTopicCommand.java PRE-CREATION 
  tools/src/main/java/org/apache/kafka/cli/command/ExitCommand.java PRE-CREATION 
  tools/src/main/java/org/apache/kafka/cli/command/ListTopicsCommand.java PRE-CREATION 
  tools/src/main/java/org/apache/kafka/cli/command/PrintHelpCommand.java PRE-CREATION 
  tools/src/main/java/org/apache/kafka/cli/command/TopicSwitchCommand.java PRE-CREATION 
  tools/src/main/java/org/apache/kafka/cli/util/StringUtils.java PRE-CREATION 

Diff: https://reviews.apache.org/r/29301/diff/


Testing
-------


Thanks,

Andrii Biletskyi