You are viewing a plain text version of this content. The canonical link for it is here.

Posted to dev@kafka.apache.org by Joel Koshy <jj...@gmail.com> on 2015/02/10 21:12:24 UTC

Re: Review Request 29647: Patch for KAFKA-1697

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



clients/src/main/java/org/apache/kafka/common/errors/NotEnoughReplicasAfterAppendException.java
<https://reviews.apache.org/r/29647/#comment117709>

    Can you clarify why this should no longer be retriable?



core/src/main/scala/kafka/server/KafkaApis.scala
<https://reviews.apache.org/r/29647/#comment117713>

    Rather than do this here in kafka-apis, how about moving this to ReplicaManager.appendMessages? I think there are a few potential benefits:
    * It keeps kafka-apis simpler as a dispatcher of requests without looking into the requests (albeit we currently break that all over the place but improved after KAFKA-1583).
    * You can just call the responseCallBack from there instead of explicitly creating the response object and sending via the request channel.
    * It may be easier to test your change - i.e., no need the full-fledged kafka-apis test. You just need to create a replicaManager with a mock zkclient and mock scheduler and test the appendMessages call.



core/src/main/scala/kafka/server/KafkaApis.scala
<https://reviews.apache.org/r/29647/#comment117712>

    Should we just do Errors.INVALID_REQUIRED_ACKS?


- Joel Koshy


On Jan. 14, 2015, 11:41 p.m., Gwen Shapira wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29647/
> -----------------------------------------------------------
> 
> (Updated Jan. 14, 2015, 11:41 p.m.)
> 
> 
> Review request for kafka.
> 
> 
> Bugs: KAFKA-1697
>     https://issues.apache.org/jira/browse/KAFKA-1697
> 
> 
> Repository: kafka
> 
> 
> Description
> -------
> 
> trivial change to add byte serializer to ProducerPerformance; patched by Jun Rao
> 
> 
> KAFKA-1723 (delta patch to fix javadoc); make the metrics name in new producer more standard; patched by Manikumar Reddy; reviewed by Jun Rao
> 
> 
> removed broker code for handling acks>1 and made NotEnoughReplicasAfterAppendException non-retriable
> 
> 
> added early handling of invalid number of acks to handler and a test
> 
> 
> Diffs
> -----
> 
>   clients/src/main/java/org/apache/kafka/common/errors/InvalidRequiredAcksException.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/errors/NotEnoughReplicasAfterAppendException.java 75c80a97e43089cb3f924a38f86d67b5a8dd2b89 
>   clients/src/main/java/org/apache/kafka/common/protocol/Errors.java 3316b6a1098311b8603a4a5893bf57b75d2e43cb 
>   core/src/main/scala/kafka/cluster/Partition.scala b230e9a1fb1a3161f4c9d164e4890a16eceb2ad4 
>   core/src/main/scala/kafka/server/KafkaApis.scala c011a1b79bd6c4e832fe7d097daacb0d647d1cd4 
>   core/src/test/scala/unit/kafka/api/RequestResponseSerializationTest.scala cd16ced5465d098be7a60498326b2a98c248f343 
>   core/src/test/scala/unit/kafka/api/testKafkaApis.scala PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/29647/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Gwen Shapira
> 
>

Re: Review Request 29647: Patch for KAFKA-1697

Posted by Gwen Shapira <gs...@cloudera.com>.


> On Feb. 10, 2015, 8:12 p.m., Joel Koshy wrote:
> > core/src/main/scala/kafka/server/KafkaApis.scala, line 166
> > <https://reviews.apache.org/r/29647/diff/2/?file=822000#file822000line166>
> >
> >     Rather than do this here in kafka-apis, how about moving this to ReplicaManager.appendMessages? I think there are a few potential benefits:
> >     * It keeps kafka-apis simpler as a dispatcher of requests without looking into the requests (albeit we currently break that all over the place but improved after KAFKA-1583).
> >     * You can just call the responseCallBack from there instead of explicitly creating the response object and sending via the request channel.
> >     * It may be easier to test your change - i.e., no need the full-fledged kafka-apis test. You just need to create a replicaManager with a mock zkclient and mock scheduler and test the appendMessages call.

Yeah. I think the code for handling errors early is a bit messy and its a question of where we prefer the mess :)

I went with handleProducerRequest() since appendMessages() already had some complex logic in it and I prefered not to complicate things farther. 
The handler code already had special cases for requiredAcks == 0, so it looked like the right place.

Anyway, the point on easier testing convinced me, will move the logic to appendMessages :)


- Gwen


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


On Jan. 14, 2015, 11:41 p.m., Gwen Shapira wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29647/
> -----------------------------------------------------------
> 
> (Updated Jan. 14, 2015, 11:41 p.m.)
> 
> 
> Review request for kafka.
> 
> 
> Bugs: KAFKA-1697
>     https://issues.apache.org/jira/browse/KAFKA-1697
> 
> 
> Repository: kafka
> 
> 
> Description
> -------
> 
> trivial change to add byte serializer to ProducerPerformance; patched by Jun Rao
> 
> 
> KAFKA-1723 (delta patch to fix javadoc); make the metrics name in new producer more standard; patched by Manikumar Reddy; reviewed by Jun Rao
> 
> 
> removed broker code for handling acks>1 and made NotEnoughReplicasAfterAppendException non-retriable
> 
> 
> added early handling of invalid number of acks to handler and a test
> 
> 
> Diffs
> -----
> 
>   clients/src/main/java/org/apache/kafka/common/errors/InvalidRequiredAcksException.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/errors/NotEnoughReplicasAfterAppendException.java 75c80a97e43089cb3f924a38f86d67b5a8dd2b89 
>   clients/src/main/java/org/apache/kafka/common/protocol/Errors.java 3316b6a1098311b8603a4a5893bf57b75d2e43cb 
>   core/src/main/scala/kafka/cluster/Partition.scala b230e9a1fb1a3161f4c9d164e4890a16eceb2ad4 
>   core/src/main/scala/kafka/server/KafkaApis.scala c011a1b79bd6c4e832fe7d097daacb0d647d1cd4 
>   core/src/test/scala/unit/kafka/api/RequestResponseSerializationTest.scala cd16ced5465d098be7a60498326b2a98c248f343 
>   core/src/test/scala/unit/kafka/api/testKafkaApis.scala PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/29647/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Gwen Shapira
> 
>

Re: Review Request 29647: Patch for KAFKA-1697

Posted by Gwen Shapira <gs...@cloudera.com>.


> On Feb. 10, 2015, 8:12 p.m., Joel Koshy wrote:
> > clients/src/main/java/org/apache/kafka/common/errors/NotEnoughReplicasAfterAppendException.java, line 24
> > <https://reviews.apache.org/r/29647/diff/2/?file=821997#file821997line24>
> >
> >     Can you clarify why this should no longer be retriable?

This error occures when the message was appended to the log and on subsequent attempt to replicate, the ISR was too small to support min.isr requirement.
A retry in this case will generate duplicates since the message was written on lead replica.

Note that this should be quite rare - we check min.isr before appending to leader, but there's always the possibility of ISR shrinking after the append was already done and before replication completed.


- Gwen


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


On Jan. 14, 2015, 11:41 p.m., Gwen Shapira wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29647/
> -----------------------------------------------------------
> 
> (Updated Jan. 14, 2015, 11:41 p.m.)
> 
> 
> Review request for kafka.
> 
> 
> Bugs: KAFKA-1697
>     https://issues.apache.org/jira/browse/KAFKA-1697
> 
> 
> Repository: kafka
> 
> 
> Description
> -------
> 
> trivial change to add byte serializer to ProducerPerformance; patched by Jun Rao
> 
> 
> KAFKA-1723 (delta patch to fix javadoc); make the metrics name in new producer more standard; patched by Manikumar Reddy; reviewed by Jun Rao
> 
> 
> removed broker code for handling acks>1 and made NotEnoughReplicasAfterAppendException non-retriable
> 
> 
> added early handling of invalid number of acks to handler and a test
> 
> 
> Diffs
> -----
> 
>   clients/src/main/java/org/apache/kafka/common/errors/InvalidRequiredAcksException.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/errors/NotEnoughReplicasAfterAppendException.java 75c80a97e43089cb3f924a38f86d67b5a8dd2b89 
>   clients/src/main/java/org/apache/kafka/common/protocol/Errors.java 3316b6a1098311b8603a4a5893bf57b75d2e43cb 
>   core/src/main/scala/kafka/cluster/Partition.scala b230e9a1fb1a3161f4c9d164e4890a16eceb2ad4 
>   core/src/main/scala/kafka/server/KafkaApis.scala c011a1b79bd6c4e832fe7d097daacb0d647d1cd4 
>   core/src/test/scala/unit/kafka/api/RequestResponseSerializationTest.scala cd16ced5465d098be7a60498326b2a98c248f343 
>   core/src/test/scala/unit/kafka/api/testKafkaApis.scala PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/29647/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Gwen Shapira
> 
>

Re: Review Request 29647: Patch for KAFKA-1697

Posted by Gwen Shapira <gs...@cloudera.com>.


> On Feb. 10, 2015, 8:12 p.m., Joel Koshy wrote:
> > core/src/main/scala/kafka/server/KafkaApis.scala, line 170
> > <https://reviews.apache.org/r/29647/diff/2/?file=822000#file822000line170>
> >
> >     Should we just do Errors.INVALID_REQUIRED_ACKS?

lol. yeah, I've no clue what I was trying to do there :)


- Gwen


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


On Jan. 14, 2015, 11:41 p.m., Gwen Shapira wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29647/
> -----------------------------------------------------------
> 
> (Updated Jan. 14, 2015, 11:41 p.m.)
> 
> 
> Review request for kafka.
> 
> 
> Bugs: KAFKA-1697
>     https://issues.apache.org/jira/browse/KAFKA-1697
> 
> 
> Repository: kafka
> 
> 
> Description
> -------
> 
> trivial change to add byte serializer to ProducerPerformance; patched by Jun Rao
> 
> 
> KAFKA-1723 (delta patch to fix javadoc); make the metrics name in new producer more standard; patched by Manikumar Reddy; reviewed by Jun Rao
> 
> 
> removed broker code for handling acks>1 and made NotEnoughReplicasAfterAppendException non-retriable
> 
> 
> added early handling of invalid number of acks to handler and a test
> 
> 
> Diffs
> -----
> 
>   clients/src/main/java/org/apache/kafka/common/errors/InvalidRequiredAcksException.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/errors/NotEnoughReplicasAfterAppendException.java 75c80a97e43089cb3f924a38f86d67b5a8dd2b89 
>   clients/src/main/java/org/apache/kafka/common/protocol/Errors.java 3316b6a1098311b8603a4a5893bf57b75d2e43cb 
>   core/src/main/scala/kafka/cluster/Partition.scala b230e9a1fb1a3161f4c9d164e4890a16eceb2ad4 
>   core/src/main/scala/kafka/server/KafkaApis.scala c011a1b79bd6c4e832fe7d097daacb0d647d1cd4 
>   core/src/test/scala/unit/kafka/api/RequestResponseSerializationTest.scala cd16ced5465d098be7a60498326b2a98c248f343 
>   core/src/test/scala/unit/kafka/api/testKafkaApis.scala PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/29647/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Gwen Shapira
> 
>