Re: Review Request 29301: Patch for KAFKA-1694

[Andrii Biletskyi]

> On Feb. 3, 2015, 7:14 p.m., Guozhang Wang wrote:
> > clients/src/main/java/org/apache/kafka/common/protocol/ApiKeys.java, line 44
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > "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
> > 
> >
> > "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
> > 
> >
> > 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
> > 
> >
> > 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

Re: Review Request 29301: Patch for KAFKA-1694

[Andrii Biletskyi]

---
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/tes

Re: Review Request 29301: Patch for KAFKA-1694

[Guozhang Wang]

> On Feb. 3, 2015, 7:14 p.m., Guozhang Wang wrote:
> > core/src/main/scala/kafka/server/TopicCommandHelper.scala, lines 1-17
> > 
> >
> > 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
> > 
> >
> > 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 
> sendAdminRequest(AbstractAdminRequest 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)" 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/#revi

Re: Review Request 29301: Patch for KAFKA-1694

[Andrii Biletskyi]

> On Feb. 3, 2015, 7:14 p.m., Guozhang Wang wrote:
> > core/src/main/scala/kafka/server/TopicCommandHelper.scala, lines 1-17
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > 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 
> sendAdminRequest(AbstractAdminRequest 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)" to 
> "sendRequest(ClientRequest)" and the caller like AlterTopicCommand.execute() 
> will be:
> 
> AlterTopicRequest alterTopicReque

Re: Review Request 29301: Patch for KAFKA-1694

[Guozhang Wang]

> 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
> > 
> >
> > 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
> > 
> >
> > 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 
> sendAdminRequest(AbstractAdminRequest 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)" 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
> > 
> >
> > 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
> > 
> >
> > 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.createOrUpdateTopicPartiti

Re: Review Request 29301: Patch for KAFKA-1694

[Andrii Biletskyi]

> 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
> > 
> >
> > 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
> > 
> >
> > 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 
sendAdminRequest(AbstractAdminRequest 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
> > 
> >
> > 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
> > 
> >
> > 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
> 
> 
> 

Re: Review Request 29301: Patch for KAFKA-1694

[Guozhang Wang]

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


Why core compilation is dependent on tools compilation?



clients/src/main/java/org/apache/kafka/common/protocol/ApiKeys.java


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


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


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


Rename to controllerStruct?



clients/src/main/java/org/apache/kafka/common/requests/admin/AbstractAdminRequest.java


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


Ditto.



config/tools-log4j.properties


Is this intentional? WARN => INFO



core/src/main/scala/kafka/api/ApiUtils.scala


"reads a list of values of type T"



core/src/main/scala/kafka/api/ApiUtils.scala


"reads a single value of type T"



core/src/main/scala/kafka/controller/ControllerChannelManager.scala


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


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



core/src/main/scala/kafka/server/TopicCommandHelper.scala


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

Re: Review Request 29301: Patch for KAFKA-1694

[Andrii Biletskyi]

---
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/commo

Re: Review Request 29301: Patch for KAFKA-1694

[Andrii Biletskyi]

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

Re: Review Request 29301: Patch for KAFKA-1694

[Andrii Biletskyi]

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

Re: Review Request 29301: Patch for KAFKA-1694

[Andrii Biletskyi]

> On Jan. 6, 2015, 8:19 a.m., Joe Stein wrote:
> > build.gradle, line 211
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > 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/co

Re: Review Request 29301: Patch for KAFKA-1694

[Andrii Biletskyi]

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

Re: Review Request 29301: Patch for KAFKA-1694

[Andrii Biletskyi]

---
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/mai

Re: Review Request 29301: Patch for KAFKA-1694

[Jeff Holoman]

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


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

Re: Review Request 29301: Patch for KAFKA-1694

[Joe Stein]

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



build.gradle


We need to have upload archive here



build.gradle


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



build.gradle


???



clients/src/main/java/org/apache/kafka/common/protocol/types/MaybeOf.java


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


why wouldn't the null size == 0?



core/src/main/scala/kafka/common/ErrorMapping.scala


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/mai

Re: Review Request 29301: Patch for KAFKA-1694

[Andrii Biletskyi]

---
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.gra

Review Request 29301: Patch for KAFKA-1694

[Andrii Biletskyi]

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