Re: Review Request 34415: Patch for KAFKA-2195

2015-06-16 Thread Andrii Biletskyi 


> On June 16, 2015, 3:52 p.m., Jun Rao wrote:
> > clients/src/main/java/org/apache/kafka/common/requests/ConsumerMetadataRequest.java,
> >  lines 49-50
> > 
> >
> > Perhaps it's better to include the request name in the error message. 
> > Ditto for the rest of the reqeusts.

Fixed.


> On June 16, 2015, 3:52 p.m., Jun Rao wrote:
> > core/src/main/scala/kafka/network/RequestChannel.scala, line 69
> > 
> >
> > To be consistent, no need to include () in calling apiVersion. Ditte 
> > below.

Done.


- Andrii


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


On June 16, 2015, 7:28 p.m., Andrii Biletskyi wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34415/
> ---
> 
> (Updated June 16, 2015, 7:28 p.m.)
> 
> 
> Review request for kafka.
> 
> 
> Bugs: KAFKA-2195
> https://issues.apache.org/jira/browse/KAFKA-2195
> 
> 
> Repository: kafka
> 
> 
> Description
> ---
> 
> KAFKA-2195 - Code review
> 
> 
> KAFKA-2195 - Code review 2
> 
> 
> KAFKA-2195 - Code review 3 (cosmetic fixes)
> 
> 
> Diffs
> -
> 
>   clients/src/main/java/org/apache/kafka/common/requests/AbstractRequest.java 
> 5e5308ec0e333179a9abbf4f3b750ea25ab57967 
>   
> clients/src/main/java/org/apache/kafka/common/requests/ConsumerMetadataRequest.java
>  04b90bfe62456a6739fe0299f1564dbd1850fe58 
>   clients/src/main/java/org/apache/kafka/common/requests/FetchRequest.java 
> 8686d83aa52e435c6adafbe9ff4bd1602281072a 
>   
> clients/src/main/java/org/apache/kafka/common/requests/HeartbeatRequest.java 
> 51d081fa296fd7c170a90a634d432067afcfe772 
>   
> clients/src/main/java/org/apache/kafka/common/requests/JoinGroupRequest.java 
> 6795682258e6b329cc3caa245b950b4dbcf0cf45 
>   
> clients/src/main/java/org/apache/kafka/common/requests/JoinGroupResponse.java 
> 8d418cd24cf6d105e9687a4a2492b8ed13738338 
>   
> clients/src/main/java/org/apache/kafka/common/requests/ListOffsetRequest.java 
> 19267ee8aad5a2f5a84cecd6fc563f00329d5035 
>   clients/src/main/java/org/apache/kafka/common/requests/MetadataRequest.java 
> 7e0ce159a2ddd041fc06116038bd5831bbca278b 
>   
> clients/src/main/java/org/apache/kafka/common/requests/MetadataResponse.java 
> 44e2ce61899889601b6aee71fa7f7ddb4a65a255 
>   
> clients/src/main/java/org/apache/kafka/common/requests/OffsetCommitRequest.java
>  8bf6cbb79a92b0878096e099ec9169d21e6d7023 
>   
> clients/src/main/java/org/apache/kafka/common/requests/OffsetFetchRequest.java
>  deec1fa480d5a5c5884a1c007b076aa64e902472 
>   clients/src/main/java/org/apache/kafka/common/requests/ProduceRequest.java 
> fabeae3083a8ea55cdacbb9568f3847ccd85bab4 
>   
> clients/src/test/java/org/apache/kafka/common/requests/RequestResponseTest.java
>  e3cc1967e407b64cc734548c19e30de700b64ba8 
>   core/src/main/scala/kafka/network/RequestChannel.scala 
> 357d8b97cb336857500213efade77950833c2096 
>   core/src/main/scala/kafka/server/KafkaApis.scala 
> d63bc18d795a6f0e6994538ca55a9a46f7fb8ffd 
> 
> Diff: https://reviews.apache.org/r/34415/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andrii Biletskyi
> 
>

Re: Review Request 34415: Patch for KAFKA-2195

2015-06-16 Thread Andrii Biletskyi 

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

(Updated June 16, 2015, 7:28 p.m.)


Review request for kafka.


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


Repository: kafka


Description (updated)
---

KAFKA-2195 - Code review


KAFKA-2195 - Code review 2


KAFKA-2195 - Code review 3 (cosmetic fixes)


Diffs (updated)
-

  clients/src/main/java/org/apache/kafka/common/requests/AbstractRequest.java 
5e5308ec0e333179a9abbf4f3b750ea25ab57967 
  
clients/src/main/java/org/apache/kafka/common/requests/ConsumerMetadataRequest.java
 04b90bfe62456a6739fe0299f1564dbd1850fe58 
  clients/src/main/java/org/apache/kafka/common/requests/FetchRequest.java 
8686d83aa52e435c6adafbe9ff4bd1602281072a 
  clients/src/main/java/org/apache/kafka/common/requests/HeartbeatRequest.java 
51d081fa296fd7c170a90a634d432067afcfe772 
  clients/src/main/java/org/apache/kafka/common/requests/JoinGroupRequest.java 
6795682258e6b329cc3caa245b950b4dbcf0cf45 
  clients/src/main/java/org/apache/kafka/common/requests/JoinGroupResponse.java 
8d418cd24cf6d105e9687a4a2492b8ed13738338 
  clients/src/main/java/org/apache/kafka/common/requests/ListOffsetRequest.java 
19267ee8aad5a2f5a84cecd6fc563f00329d5035 
  clients/src/main/java/org/apache/kafka/common/requests/MetadataRequest.java 
7e0ce159a2ddd041fc06116038bd5831bbca278b 
  clients/src/main/java/org/apache/kafka/common/requests/MetadataResponse.java 
44e2ce61899889601b6aee71fa7f7ddb4a65a255 
  
clients/src/main/java/org/apache/kafka/common/requests/OffsetCommitRequest.java 
8bf6cbb79a92b0878096e099ec9169d21e6d7023 
  
clients/src/main/java/org/apache/kafka/common/requests/OffsetFetchRequest.java 
deec1fa480d5a5c5884a1c007b076aa64e902472 
  clients/src/main/java/org/apache/kafka/common/requests/ProduceRequest.java 
fabeae3083a8ea55cdacbb9568f3847ccd85bab4 
  
clients/src/test/java/org/apache/kafka/common/requests/RequestResponseTest.java 
e3cc1967e407b64cc734548c19e30de700b64ba8 
  core/src/main/scala/kafka/network/RequestChannel.scala 
357d8b97cb336857500213efade77950833c2096 
  core/src/main/scala/kafka/server/KafkaApis.scala 
d63bc18d795a6f0e6994538ca55a9a46f7fb8ffd 

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


Testing
---


Thanks,

Andrii Biletskyi

Re: Review Request 34415: Patch for KAFKA-2195

2015-06-16 Thread Jun Rao 

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


Thanks for the latest patch. A couple more minor comments.


clients/src/main/java/org/apache/kafka/common/requests/ConsumerMetadataRequest.java
 (lines 49 - 50)


Perhaps it's better to include the request name in the error message. Ditto 
for the rest of the reqeusts.



core/src/main/scala/kafka/network/RequestChannel.scala (line 69)


To be consistent, no need to include () in calling apiVersion. Ditte below.


- Jun Rao


On June 15, 2015, 6:55 a.m., Andrii Biletskyi wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34415/
> ---
> 
> (Updated June 15, 2015, 6:55 a.m.)
> 
> 
> Review request for kafka.
> 
> 
> Bugs: KAFKA-2195
> https://issues.apache.org/jira/browse/KAFKA-2195
> 
> 
> Repository: kafka
> 
> 
> Description
> ---
> 
> KAFKA-2195 - Code review
> 
> 
> KAFKA-2195 - Code review 2
> 
> 
> Diffs
> -
> 
>   clients/src/main/java/org/apache/kafka/common/requests/AbstractRequest.java 
> 5e5308ec0e333179a9abbf4f3b750ea25ab57967 
>   
> clients/src/main/java/org/apache/kafka/common/requests/ConsumerMetadataRequest.java
>  04b90bfe62456a6739fe0299f1564dbd1850fe58 
>   clients/src/main/java/org/apache/kafka/common/requests/FetchRequest.java 
> 8686d83aa52e435c6adafbe9ff4bd1602281072a 
>   
> clients/src/main/java/org/apache/kafka/common/requests/HeartbeatRequest.java 
> 51d081fa296fd7c170a90a634d432067afcfe772 
>   
> clients/src/main/java/org/apache/kafka/common/requests/JoinGroupRequest.java 
> 6795682258e6b329cc3caa245b950b4dbcf0cf45 
>   
> clients/src/main/java/org/apache/kafka/common/requests/JoinGroupResponse.java 
> 8d418cd24cf6d105e9687a4a2492b8ed13738338 
>   
> clients/src/main/java/org/apache/kafka/common/requests/ListOffsetRequest.java 
> 19267ee8aad5a2f5a84cecd6fc563f00329d5035 
>   clients/src/main/java/org/apache/kafka/common/requests/MetadataRequest.java 
> 7e0ce159a2ddd041fc06116038bd5831bbca278b 
>   
> clients/src/main/java/org/apache/kafka/common/requests/MetadataResponse.java 
> 44e2ce61899889601b6aee71fa7f7ddb4a65a255 
>   
> clients/src/main/java/org/apache/kafka/common/requests/OffsetCommitRequest.java
>  8bf6cbb79a92b0878096e099ec9169d21e6d7023 
>   
> clients/src/main/java/org/apache/kafka/common/requests/OffsetFetchRequest.java
>  deec1fa480d5a5c5884a1c007b076aa64e902472 
>   clients/src/main/java/org/apache/kafka/common/requests/ProduceRequest.java 
> fabeae3083a8ea55cdacbb9568f3847ccd85bab4 
>   
> clients/src/test/java/org/apache/kafka/common/requests/RequestResponseTest.java
>  e3cc1967e407b64cc734548c19e30de700b64ba8 
>   core/src/main/scala/kafka/network/RequestChannel.scala 
> 357d8b97cb336857500213efade77950833c2096 
>   core/src/main/scala/kafka/server/KafkaApis.scala 
> d63bc18d795a6f0e6994538ca55a9a46f7fb8ffd 
> 
> Diff: https://reviews.apache.org/r/34415/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andrii Biletskyi
> 
>

Re: Review Request 34415: Patch for KAFKA-2195

2015-06-14 Thread Andrii Biletskyi 

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

(Updated June 15, 2015, 6:55 a.m.)


Review request for kafka.


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


Repository: kafka


Description (updated)
---

KAFKA-2195 - Code review


KAFKA-2195 - Code review 2


Diffs (updated)
-

  clients/src/main/java/org/apache/kafka/common/requests/AbstractRequest.java 
5e5308ec0e333179a9abbf4f3b750ea25ab57967 
  
clients/src/main/java/org/apache/kafka/common/requests/ConsumerMetadataRequest.java
 04b90bfe62456a6739fe0299f1564dbd1850fe58 
  clients/src/main/java/org/apache/kafka/common/requests/FetchRequest.java 
8686d83aa52e435c6adafbe9ff4bd1602281072a 
  clients/src/main/java/org/apache/kafka/common/requests/HeartbeatRequest.java 
51d081fa296fd7c170a90a634d432067afcfe772 
  clients/src/main/java/org/apache/kafka/common/requests/JoinGroupRequest.java 
6795682258e6b329cc3caa245b950b4dbcf0cf45 
  clients/src/main/java/org/apache/kafka/common/requests/JoinGroupResponse.java 
8d418cd24cf6d105e9687a4a2492b8ed13738338 
  clients/src/main/java/org/apache/kafka/common/requests/ListOffsetRequest.java 
19267ee8aad5a2f5a84cecd6fc563f00329d5035 
  clients/src/main/java/org/apache/kafka/common/requests/MetadataRequest.java 
7e0ce159a2ddd041fc06116038bd5831bbca278b 
  clients/src/main/java/org/apache/kafka/common/requests/MetadataResponse.java 
44e2ce61899889601b6aee71fa7f7ddb4a65a255 
  
clients/src/main/java/org/apache/kafka/common/requests/OffsetCommitRequest.java 
8bf6cbb79a92b0878096e099ec9169d21e6d7023 
  
clients/src/main/java/org/apache/kafka/common/requests/OffsetFetchRequest.java 
deec1fa480d5a5c5884a1c007b076aa64e902472 
  clients/src/main/java/org/apache/kafka/common/requests/ProduceRequest.java 
fabeae3083a8ea55cdacbb9568f3847ccd85bab4 
  
clients/src/test/java/org/apache/kafka/common/requests/RequestResponseTest.java 
e3cc1967e407b64cc734548c19e30de700b64ba8 
  core/src/main/scala/kafka/network/RequestChannel.scala 
357d8b97cb336857500213efade77950833c2096 
  core/src/main/scala/kafka/server/KafkaApis.scala 
d63bc18d795a6f0e6994538ca55a9a46f7fb8ffd 

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


Testing
---


Thanks,

Andrii Biletskyi

Re: Review Request 34415: Patch for KAFKA-2195

2015-05-26 Thread Jun Rao 

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


Thanks for the patch. Just one more comment blow.


clients/src/main/java/org/apache/kafka/common/requests/MetadataResponse.java


Could we remove this constructor and just keep the one with errors?


- Jun Rao


On May 24, 2015, 7:49 p.m., Andrii Biletskyi wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34415/
> ---
> 
> (Updated May 24, 2015, 7:49 p.m.)
> 
> 
> Review request for kafka.
> 
> 
> Bugs: KAFKA-2195
> https://issues.apache.org/jira/browse/KAFKA-2195
> 
> 
> Repository: kafka
> 
> 
> Description
> ---
> 
> KAFKA-2195 - Code review
> 
> 
> Diffs
> -
> 
>   clients/src/main/java/org/apache/kafka/common/requests/AbstractRequest.java 
> 5e5308ec0e333179a9abbf4f3b750ea25ab57967 
>   
> clients/src/main/java/org/apache/kafka/common/requests/ConsumerMetadataRequest.java
>  04b90bfe62456a6739fe0299f1564dbd1850fe58 
>   clients/src/main/java/org/apache/kafka/common/requests/FetchRequest.java 
> 8686d83aa52e435c6adafbe9ff4bd1602281072a 
>   
> clients/src/main/java/org/apache/kafka/common/requests/HeartbeatRequest.java 
> 51d081fa296fd7c170a90a634d432067afcfe772 
>   
> clients/src/main/java/org/apache/kafka/common/requests/JoinGroupRequest.java 
> 6795682258e6b329cc3caa245b950b4dbcf0cf45 
>   
> clients/src/main/java/org/apache/kafka/common/requests/JoinGroupResponse.java 
> fd9c545c99058ad3fbe3b2c55ea8b6ea002f5a51 
>   
> clients/src/main/java/org/apache/kafka/common/requests/ListOffsetRequest.java 
> 19267ee8aad5a2f5a84cecd6fc563f00329d5035 
>   clients/src/main/java/org/apache/kafka/common/requests/MetadataRequest.java 
> 7e0ce159a2ddd041fc06116038bd5831bbca278b 
>   
> clients/src/main/java/org/apache/kafka/common/requests/MetadataResponse.java 
> 44e2ce61899889601b6aee71fa7f7ddb4a65a255 
>   
> clients/src/main/java/org/apache/kafka/common/requests/OffsetCommitRequest.java
>  8bf6cbb79a92b0878096e099ec9169d21e6d7023 
>   
> clients/src/main/java/org/apache/kafka/common/requests/OffsetFetchRequest.java
>  deec1fa480d5a5c5884a1c007b076aa64e902472 
>   clients/src/main/java/org/apache/kafka/common/requests/ProduceRequest.java 
> fabeae3083a8ea55cdacbb9568f3847ccd85bab4 
>   
> clients/src/test/java/org/apache/kafka/common/requests/RequestResponseTest.java
>  e3cc1967e407b64cc734548c19e30de700b64ba8 
>   core/src/main/scala/kafka/network/RequestChannel.scala 
> 1d0024c8f0c2ab0efa6d8cfca6455877a6ed8026 
>   core/src/main/scala/kafka/server/KafkaApis.scala 
> 387e387998fc3a6c9cb585dab02b5f77b0381fbf 
> 
> Diff: https://reviews.apache.org/r/34415/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andrii Biletskyi
> 
>

Re: Review Request 34415: Patch for KAFKA-2195

2015-05-24 Thread Andrii Biletskyi 

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

(Updated May 24, 2015, 7:49 p.m.)


Review request for kafka.


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


Repository: kafka


Description (updated)
---

KAFKA-2195 - Code review


Diffs (updated)
-

  clients/src/main/java/org/apache/kafka/common/requests/AbstractRequest.java 
5e5308ec0e333179a9abbf4f3b750ea25ab57967 
  
clients/src/main/java/org/apache/kafka/common/requests/ConsumerMetadataRequest.java
 04b90bfe62456a6739fe0299f1564dbd1850fe58 
  clients/src/main/java/org/apache/kafka/common/requests/FetchRequest.java 
8686d83aa52e435c6adafbe9ff4bd1602281072a 
  clients/src/main/java/org/apache/kafka/common/requests/HeartbeatRequest.java 
51d081fa296fd7c170a90a634d432067afcfe772 
  clients/src/main/java/org/apache/kafka/common/requests/JoinGroupRequest.java 
6795682258e6b329cc3caa245b950b4dbcf0cf45 
  clients/src/main/java/org/apache/kafka/common/requests/JoinGroupResponse.java 
fd9c545c99058ad3fbe3b2c55ea8b6ea002f5a51 
  clients/src/main/java/org/apache/kafka/common/requests/ListOffsetRequest.java 
19267ee8aad5a2f5a84cecd6fc563f00329d5035 
  clients/src/main/java/org/apache/kafka/common/requests/MetadataRequest.java 
7e0ce159a2ddd041fc06116038bd5831bbca278b 
  clients/src/main/java/org/apache/kafka/common/requests/MetadataResponse.java 
44e2ce61899889601b6aee71fa7f7ddb4a65a255 
  
clients/src/main/java/org/apache/kafka/common/requests/OffsetCommitRequest.java 
8bf6cbb79a92b0878096e099ec9169d21e6d7023 
  
clients/src/main/java/org/apache/kafka/common/requests/OffsetFetchRequest.java 
deec1fa480d5a5c5884a1c007b076aa64e902472 
  clients/src/main/java/org/apache/kafka/common/requests/ProduceRequest.java 
fabeae3083a8ea55cdacbb9568f3847ccd85bab4 
  
clients/src/test/java/org/apache/kafka/common/requests/RequestResponseTest.java 
e3cc1967e407b64cc734548c19e30de700b64ba8 
  core/src/main/scala/kafka/network/RequestChannel.scala 
1d0024c8f0c2ab0efa6d8cfca6455877a6ed8026 
  core/src/main/scala/kafka/server/KafkaApis.scala 
387e387998fc3a6c9cb585dab02b5f77b0381fbf 

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


Testing
---


Thanks,

Andrii Biletskyi

Re: Review Request 34415: Patch for KAFKA-2195

2015-05-22 Thread Andrii Biletskyi 


> On May 22, 2015, 4:37 p.m., Jun Rao wrote:
> > clients/src/main/java/org/apache/kafka/common/requests/AbstractRequest.java,
> >  lines 45-62
> > 
> >
> > Could we change all requests to use parse(buffer, versionId)?

Agree.


> On May 22, 2015, 4:37 p.m., Jun Rao wrote:
> > clients/src/main/java/org/apache/kafka/common/requests/AbstractRequest.java,
> >  lines 35-38
> > 
> >
> > Could we just remove this api and use the new one with versionId?

Agree, makes sense for me.


> On May 22, 2015, 4:37 p.m., Jun Rao wrote:
> > clients/src/main/java/org/apache/kafka/common/requests/ConsumerMetadataRequest.java,
> >  line 49
> > 
> >
> > It would be better if we can make this a complete sentence. Sth like 
> > "Version x for ConsumerMetadataResponse is not valid. Valid versions are 0 
> > to 1".

Okay, will fix the error message.


> On May 22, 2015, 4:37 p.m., Jun Rao wrote:
> > clients/src/main/java/org/apache/kafka/common/requests/MetadataResponse.java,
> >  lines 70-76
> > 
> >
> > For responses, we actually don't have to maintain the compatibility of 
> > the all constructors. The reason is that a client always constructs a 
> > response from a struct. We can freely change the signature of other 
> > constructors since only the server will use them.

Okay, I'll remove deprecation notice.


- Andrii


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


On May 19, 2015, 4:09 p.m., Andrii Biletskyi wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34415/
> ---
> 
> (Updated May 19, 2015, 4:09 p.m.)
> 
> 
> Review request for kafka.
> 
> 
> Bugs: KAFKA-2195
> https://issues.apache.org/jira/browse/KAFKA-2195
> 
> 
> Repository: kafka
> 
> 
> Description
> ---
> 
> KAFKA-2195 - Add versionId to AbstractRequest.getErrorResponse and 
> AbstractRequest.getRequest
> 
> 
> Diffs
> -
> 
>   clients/src/main/java/org/apache/kafka/common/requests/AbstractRequest.java 
> 5e5308ec0e333179a9abbf4f3b750ea25ab57967 
>   
> clients/src/main/java/org/apache/kafka/common/requests/ConsumerMetadataRequest.java
>  04b90bfe62456a6739fe0299f1564dbd1850fe58 
>   clients/src/main/java/org/apache/kafka/common/requests/FetchRequest.java 
> 8686d83aa52e435c6adafbe9ff4bd1602281072a 
>   
> clients/src/main/java/org/apache/kafka/common/requests/HeartbeatRequest.java 
> 51d081fa296fd7c170a90a634d432067afcfe772 
>   
> clients/src/main/java/org/apache/kafka/common/requests/JoinGroupRequest.java 
> 6795682258e6b329cc3caa245b950b4dbcf0cf45 
>   
> clients/src/main/java/org/apache/kafka/common/requests/JoinGroupResponse.java 
> fd9c545c99058ad3fbe3b2c55ea8b6ea002f5a51 
>   
> clients/src/main/java/org/apache/kafka/common/requests/ListOffsetRequest.java 
> 19267ee8aad5a2f5a84cecd6fc563f00329d5035 
>   clients/src/main/java/org/apache/kafka/common/requests/MetadataRequest.java 
> 7e0ce159a2ddd041fc06116038bd5831bbca278b 
>   
> clients/src/main/java/org/apache/kafka/common/requests/MetadataResponse.java 
> 44e2ce61899889601b6aee71fa7f7ddb4a65a255 
>   
> clients/src/main/java/org/apache/kafka/common/requests/OffsetCommitRequest.java
>  8bf6cbb79a92b0878096e099ec9169d21e6d7023 
>   
> clients/src/main/java/org/apache/kafka/common/requests/OffsetFetchRequest.java
>  deec1fa480d5a5c5884a1c007b076aa64e902472 
>   clients/src/main/java/org/apache/kafka/common/requests/ProduceRequest.java 
> fabeae3083a8ea55cdacbb9568f3847ccd85bab4 
>   
> clients/src/test/java/org/apache/kafka/common/requests/RequestResponseTest.java
>  e3cc1967e407b64cc734548c19e30de700b64ba8 
>   core/src/main/scala/kafka/network/RequestChannel.scala 
> 1d0024c8f0c2ab0efa6d8cfca6455877a6ed8026 
>   core/src/main/scala/kafka/server/KafkaApis.scala 
> 387e387998fc3a6c9cb585dab02b5f77b0381fbf 
> 
> Diff: https://reviews.apache.org/r/34415/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andrii Biletskyi
> 
>

Re: Review Request 34415: Patch for KAFKA-2195

2015-05-22 Thread Jun Rao 

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


Thanks for the patch. A few comments below.


clients/src/main/java/org/apache/kafka/common/requests/AbstractRequest.java


Could we just remove this api and use the new one with versionId?



clients/src/main/java/org/apache/kafka/common/requests/AbstractRequest.java


Could we change all requests to use parse(buffer, versionId)?



clients/src/main/java/org/apache/kafka/common/requests/ConsumerMetadataRequest.java


It would be better if we can make this a complete sentence. Sth like 
"Version x for ConsumerMetadataResponse is not valid. Valid versions are 0 to 
1".



clients/src/main/java/org/apache/kafka/common/requests/MetadataResponse.java


For responses, we actually don't have to maintain the compatibility of the 
all constructors. The reason is that a client always constructs a response from 
a struct. We can freely change the signature of other constructors since only 
the server will use them.


- Jun Rao


On May 19, 2015, 4:09 p.m., Andrii Biletskyi wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34415/
> ---
> 
> (Updated May 19, 2015, 4:09 p.m.)
> 
> 
> Review request for kafka.
> 
> 
> Bugs: KAFKA-2195
> https://issues.apache.org/jira/browse/KAFKA-2195
> 
> 
> Repository: kafka
> 
> 
> Description
> ---
> 
> KAFKA-2195 - Add versionId to AbstractRequest.getErrorResponse and 
> AbstractRequest.getRequest
> 
> 
> Diffs
> -
> 
>   clients/src/main/java/org/apache/kafka/common/requests/AbstractRequest.java 
> 5e5308ec0e333179a9abbf4f3b750ea25ab57967 
>   
> clients/src/main/java/org/apache/kafka/common/requests/ConsumerMetadataRequest.java
>  04b90bfe62456a6739fe0299f1564dbd1850fe58 
>   clients/src/main/java/org/apache/kafka/common/requests/FetchRequest.java 
> 8686d83aa52e435c6adafbe9ff4bd1602281072a 
>   
> clients/src/main/java/org/apache/kafka/common/requests/HeartbeatRequest.java 
> 51d081fa296fd7c170a90a634d432067afcfe772 
>   
> clients/src/main/java/org/apache/kafka/common/requests/JoinGroupRequest.java 
> 6795682258e6b329cc3caa245b950b4dbcf0cf45 
>   
> clients/src/main/java/org/apache/kafka/common/requests/JoinGroupResponse.java 
> fd9c545c99058ad3fbe3b2c55ea8b6ea002f5a51 
>   
> clients/src/main/java/org/apache/kafka/common/requests/ListOffsetRequest.java 
> 19267ee8aad5a2f5a84cecd6fc563f00329d5035 
>   clients/src/main/java/org/apache/kafka/common/requests/MetadataRequest.java 
> 7e0ce159a2ddd041fc06116038bd5831bbca278b 
>   
> clients/src/main/java/org/apache/kafka/common/requests/MetadataResponse.java 
> 44e2ce61899889601b6aee71fa7f7ddb4a65a255 
>   
> clients/src/main/java/org/apache/kafka/common/requests/OffsetCommitRequest.java
>  8bf6cbb79a92b0878096e099ec9169d21e6d7023 
>   
> clients/src/main/java/org/apache/kafka/common/requests/OffsetFetchRequest.java
>  deec1fa480d5a5c5884a1c007b076aa64e902472 
>   clients/src/main/java/org/apache/kafka/common/requests/ProduceRequest.java 
> fabeae3083a8ea55cdacbb9568f3847ccd85bab4 
>   
> clients/src/test/java/org/apache/kafka/common/requests/RequestResponseTest.java
>  e3cc1967e407b64cc734548c19e30de700b64ba8 
>   core/src/main/scala/kafka/network/RequestChannel.scala 
> 1d0024c8f0c2ab0efa6d8cfca6455877a6ed8026 
>   core/src/main/scala/kafka/server/KafkaApis.scala 
> 387e387998fc3a6c9cb585dab02b5f77b0381fbf 
> 
> Diff: https://reviews.apache.org/r/34415/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andrii Biletskyi
> 
>

Review Request 34415: Patch for KAFKA-2195

2015-05-19 Thread Andrii Biletskyi 

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

Review request for kafka.


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


Repository: kafka


Description
---

KAFKA-2195 - Add versionId to AbstractRequest.getErrorResponse and 
AbstractRequest.getRequest


Diffs
-

  clients/src/main/java/org/apache/kafka/common/requests/AbstractRequest.java 
5e5308ec0e333179a9abbf4f3b750ea25ab57967 
  
clients/src/main/java/org/apache/kafka/common/requests/ConsumerMetadataRequest.java
 04b90bfe62456a6739fe0299f1564dbd1850fe58 
  clients/src/main/java/org/apache/kafka/common/requests/FetchRequest.java 
8686d83aa52e435c6adafbe9ff4bd1602281072a 
  clients/src/main/java/org/apache/kafka/common/requests/HeartbeatRequest.java 
51d081fa296fd7c170a90a634d432067afcfe772 
  clients/src/main/java/org/apache/kafka/common/requests/JoinGroupRequest.java 
6795682258e6b329cc3caa245b950b4dbcf0cf45 
  clients/src/main/java/org/apache/kafka/common/requests/JoinGroupResponse.java 
fd9c545c99058ad3fbe3b2c55ea8b6ea002f5a51 
  clients/src/main/java/org/apache/kafka/common/requests/ListOffsetRequest.java 
19267ee8aad5a2f5a84cecd6fc563f00329d5035 
  clients/src/main/java/org/apache/kafka/common/requests/MetadataRequest.java 
7e0ce159a2ddd041fc06116038bd5831bbca278b 
  clients/src/main/java/org/apache/kafka/common/requests/MetadataResponse.java 
44e2ce61899889601b6aee71fa7f7ddb4a65a255 
  
clients/src/main/java/org/apache/kafka/common/requests/OffsetCommitRequest.java 
8bf6cbb79a92b0878096e099ec9169d21e6d7023 
  
clients/src/main/java/org/apache/kafka/common/requests/OffsetFetchRequest.java 
deec1fa480d5a5c5884a1c007b076aa64e902472 
  clients/src/main/java/org/apache/kafka/common/requests/ProduceRequest.java 
fabeae3083a8ea55cdacbb9568f3847ccd85bab4 
  
clients/src/test/java/org/apache/kafka/common/requests/RequestResponseTest.java 
e3cc1967e407b64cc734548c19e30de700b64ba8 
  core/src/main/scala/kafka/network/RequestChannel.scala 
1d0024c8f0c2ab0efa6d8cfca6455877a6ed8026 
  core/src/main/scala/kafka/server/KafkaApis.scala 
387e387998fc3a6c9cb585dab02b5f77b0381fbf 

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


Testing
---


Thanks,

Andrii Biletskyi