Re: Review Request 24992: Patch for KAFKA-1609

2014-08-22 Thread Dong Lin 


> On Aug. 22, 2014, 10:39 p.m., Jun Rao wrote:
> > clients/src/main/java/org/apache/kafka/common/requests/MetadataResponse.java,
> >  line 121
> > 
> >
> > Would it be simpler to just don't do the test on partError? The rest of 
> > the code can deal with a null leader already. It's probably a bit weird to 
> > ignore a partition whose leader is not available. For example, if a message 
> > has to go to a partition (e.g., keyed message), we want to know that 
> > partition whether the leader is available or not.

Sure! The patch is updated.


- Dong


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


On Aug. 22, 2014, 11:22 p.m., Dong Lin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24992/
> ---
> 
> (Updated Aug. 22, 2014, 11:22 p.m.)
> 
> 
> Review request for kafka.
> 
> 
> Bugs: KAFKA-1609
> https://issues.apache.org/jira/browse/KAFKA-1609
> 
> 
> Repository: kafka
> 
> 
> Description
> ---
> 
> KAFKA-1609; New producer metadata response handling should not exclude a 
> PartitionInfo when there is partition error.
> 
> 
> Diffs
> -
> 
>   
> clients/src/main/java/org/apache/kafka/common/requests/MetadataResponse.java 
> 7d90fceffd4a92460c616f8bacd78057a38a3c16 
> 
> Diff: https://reviews.apache.org/r/24992/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Dong Lin
> 
>

Re: Review Request 24992: Patch for KAFKA-1609

2014-08-22 Thread Dong Lin 

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

(Updated Aug. 22, 2014, 11:22 p.m.)


Review request for kafka.


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


Repository: kafka


Description (updated)
---

KAFKA-1609; New producer metadata response handling should not exclude a 
PartitionInfo when there is partition error.


Diffs (updated)
-

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

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


Testing
---


Thanks,

Dong Lin

Re: Review Request 24992: Patch for KAFKA-1609

2014-08-22 Thread Jun Rao 

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



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


Would it be simpler to just don't do the test on partError? The rest of the 
code can deal with a null leader already. It's probably a bit weird to ignore a 
partition whose leader is not available. For example, if a message has to go to 
a partition (e.g., keyed message), we want to know that partition whether the 
leader is available or not.


- Jun Rao


On Aug. 22, 2014, 8:04 p.m., Dong Lin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24992/
> ---
> 
> (Updated Aug. 22, 2014, 8:04 p.m.)
> 
> 
> Review request for kafka.
> 
> 
> Bugs: KAFKA-1609
> https://issues.apache.org/jira/browse/KAFKA-1609
> 
> 
> Repository: kafka
> 
> 
> Description
> ---
> 
> KAFKA-1609; New producer metadata response handling should only exclude a 
> PartitionInfo when its error is LEADER_NOT_AVAILABLE
> 
> 
> Diffs
> -
> 
>   
> clients/src/main/java/org/apache/kafka/common/requests/MetadataResponse.java 
> 7d90fceffd4a92460c616f8bacd78057a38a3c16 
> 
> Diff: https://reviews.apache.org/r/24992/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Dong Lin
> 
>

Re: Review Request 24992: Patch for KAFKA-1609

2014-08-22 Thread Guozhang Wang 

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

Ship it!


Ship It!

- Guozhang Wang


On Aug. 22, 2014, 8:04 p.m., Dong Lin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24992/
> ---
> 
> (Updated Aug. 22, 2014, 8:04 p.m.)
> 
> 
> Review request for kafka.
> 
> 
> Bugs: KAFKA-1609
> https://issues.apache.org/jira/browse/KAFKA-1609
> 
> 
> Repository: kafka
> 
> 
> Description
> ---
> 
> KAFKA-1609; New producer metadata response handling should only exclude a 
> PartitionInfo when its error is LEADER_NOT_AVAILABLE
> 
> 
> Diffs
> -
> 
>   
> clients/src/main/java/org/apache/kafka/common/requests/MetadataResponse.java 
> 7d90fceffd4a92460c616f8bacd78057a38a3c16 
> 
> Diff: https://reviews.apache.org/r/24992/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Dong Lin
> 
>

Re: Review Request 24992: Patch for KAFKA-1609

2014-08-22 Thread Dong Lin 


> On Aug. 22, 2014, 8:50 p.m., Guozhang Wang wrote:
> > clients/src/main/java/org/apache/kafka/common/requests/MetadataResponse.java,
> >  line 121
> > 
> >
> > Is this possilbe that the reponse can also contain other exceptions 
> > like UNKNOWN_TOPIC_OR_PARTITION now or in the future? If yes we also need 
> > to exlude them also.

I think the UNKNOWN_TOPIC_OR_PARTITION will be captured in topic error code. 
From what I learned from source code, currently metadata response only returns 
LEADER_NOT_AVAILABLE or REPLICA_NOT_AVAILABLE as parition error code.


- Dong


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


On Aug. 22, 2014, 8:04 p.m., Dong Lin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24992/
> ---
> 
> (Updated Aug. 22, 2014, 8:04 p.m.)
> 
> 
> Review request for kafka.
> 
> 
> Bugs: KAFKA-1609
> https://issues.apache.org/jira/browse/KAFKA-1609
> 
> 
> Repository: kafka
> 
> 
> Description
> ---
> 
> KAFKA-1609; New producer metadata response handling should only exclude a 
> PartitionInfo when its error is LEADER_NOT_AVAILABLE
> 
> 
> Diffs
> -
> 
>   
> clients/src/main/java/org/apache/kafka/common/requests/MetadataResponse.java 
> 7d90fceffd4a92460c616f8bacd78057a38a3c16 
> 
> Diff: https://reviews.apache.org/r/24992/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Dong Lin
> 
>

Re: Review Request 24992: Patch for KAFKA-1609

2014-08-22 Thread Dong Lin 


> On Aug. 22, 2014, 8:56 p.m., Joel Koshy wrote:
> > clients/src/main/java/org/apache/kafka/common/requests/MetadataResponse.java,
> >  line 121
> > 
> >
> > Guozhang, an unknown topic would be captured at the topic-error level 
> > (above) no? Unknown partition should not happen since we don't support 
> > partition deletion and anyway currently don't query metadata at the 
> > partition-level.

I agree.


- Dong


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


On Aug. 22, 2014, 8:04 p.m., Dong Lin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24992/
> ---
> 
> (Updated Aug. 22, 2014, 8:04 p.m.)
> 
> 
> Review request for kafka.
> 
> 
> Bugs: KAFKA-1609
> https://issues.apache.org/jira/browse/KAFKA-1609
> 
> 
> Repository: kafka
> 
> 
> Description
> ---
> 
> KAFKA-1609; New producer metadata response handling should only exclude a 
> PartitionInfo when its error is LEADER_NOT_AVAILABLE
> 
> 
> Diffs
> -
> 
>   
> clients/src/main/java/org/apache/kafka/common/requests/MetadataResponse.java 
> 7d90fceffd4a92460c616f8bacd78057a38a3c16 
> 
> Diff: https://reviews.apache.org/r/24992/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Dong Lin
> 
>

Re: Review Request 24992: Patch for KAFKA-1609

2014-08-22 Thread Joel Koshy 

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



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


Guozhang, an unknown topic would be captured at the topic-error level 
(above) no? Unknown partition should not happen since we don't support 
partition deletion and anyway currently don't query metadata at the 
partition-level.


- Joel Koshy


On Aug. 22, 2014, 8:04 p.m., Dong Lin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24992/
> ---
> 
> (Updated Aug. 22, 2014, 8:04 p.m.)
> 
> 
> Review request for kafka.
> 
> 
> Bugs: KAFKA-1609
> https://issues.apache.org/jira/browse/KAFKA-1609
> 
> 
> Repository: kafka
> 
> 
> Description
> ---
> 
> KAFKA-1609; New producer metadata response handling should only exclude a 
> PartitionInfo when its error is LEADER_NOT_AVAILABLE
> 
> 
> Diffs
> -
> 
>   
> clients/src/main/java/org/apache/kafka/common/requests/MetadataResponse.java 
> 7d90fceffd4a92460c616f8bacd78057a38a3c16 
> 
> Diff: https://reviews.apache.org/r/24992/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Dong Lin
> 
>

Re: Review Request 24992: Patch for KAFKA-1609

2014-08-22 Thread Guozhang Wang 

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



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


Is this possilbe that the reponse can also contain other exceptions like 
UNKNOWN_TOPIC_OR_PARTITION now or in the future? If yes we also need to exlude 
them also.


- Guozhang Wang


On Aug. 22, 2014, 8:04 p.m., Dong Lin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24992/
> ---
> 
> (Updated Aug. 22, 2014, 8:04 p.m.)
> 
> 
> Review request for kafka.
> 
> 
> Bugs: KAFKA-1609
> https://issues.apache.org/jira/browse/KAFKA-1609
> 
> 
> Repository: kafka
> 
> 
> Description
> ---
> 
> KAFKA-1609; New producer metadata response handling should only exclude a 
> PartitionInfo when its error is LEADER_NOT_AVAILABLE
> 
> 
> Diffs
> -
> 
>   
> clients/src/main/java/org/apache/kafka/common/requests/MetadataResponse.java 
> 7d90fceffd4a92460c616f8bacd78057a38a3c16 
> 
> Diff: https://reviews.apache.org/r/24992/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Dong Lin
> 
>

Re: Review Request 24992: Patch for KAFKA-1609

2014-08-22 Thread Dong Lin 

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

(Updated Aug. 22, 2014, 8:04 p.m.)


Review request for kafka.


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


Repository: kafka


Description (updated)
---

KAFKA-1609; New producer metadata response handling should only exclude a 
PartitionInfo when its error is LEADER_NOT_AVAILABLE


Diffs (updated)
-

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

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


Testing
---


Thanks,

Dong Lin

Re: Review Request 24992: Patch for KAFKA-1609

2014-08-22 Thread Dong Lin 

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

(Updated Aug. 22, 2014, 8:02 p.m.)


Review request for kafka.


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


Repository: kafka


Description (updated)
---

KAFKA-1609; New producer metadata response handling should only exclude a 
PartitionInfo when its error is LEADER_NOT_AVAILABLE


Diffs
-

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

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


Testing
---


Thanks,

Dong Lin

Review Request 24992: Patch for KAFKA-1609

2014-08-22 Thread Dong Lin 

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

Review request for kafka.


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


Repository: kafka


Description
---

KAFKA-1609; Metadata response should only exclude a PartitionInfo when its 
error is LEADER_NOT_AVAILABLE


Diffs
-

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

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


Testing
---


Thanks,

Dong Lin