You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@kafka.apache.org by Gwen Shapira <gs...@cloudera.com> on 2015/01/07 01:20:09 UTC
Review Request 29647: Patch for KAFKA-1697
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29647/
-----------------------------------------------------------
Review request for kafka.
Bugs: KAFKA-1697
https://issues.apache.org/jira/browse/KAFKA-1697
Repository: kafka
Description
-------
removed broker code for handling acks>1 and made NotEnoughReplicasAfterAppendException non-retriable
Diffs
-----
clients/src/main/java/org/apache/kafka/common/errors/NotEnoughReplicasAfterAppendException.java 75c80a97e43089cb3f924a38f86d67b5a8dd2b89
core/src/main/scala/kafka/cluster/Partition.scala b230e9a1fb1a3161f4c9d164e4890a16eceb2ad4
Diff: https://reviews.apache.org/r/29647/diff/
Testing
-------
Thanks,
Gwen Shapira
Re: Review Request 29647: Patch for KAFKA-1697
Posted by Neha Narkhede <ne...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29647/#review67489
-----------------------------------------------------------
This seems like a small change, but it is not since it changes the behavior of a user facing API. I'd suggest you circulate this proposal through the dev mailing list to solicite feedback since the mailing list has more visibility than reviewboard.
- Neha Narkhede
On Jan. 7, 2015, 12:20 a.m., Gwen Shapira wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29647/
> -----------------------------------------------------------
>
> (Updated Jan. 7, 2015, 12:20 a.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1697
> https://issues.apache.org/jira/browse/KAFKA-1697
>
>
> Repository: kafka
>
>
> Description
> -------
>
> removed broker code for handling acks>1 and made NotEnoughReplicasAfterAppendException non-retriable
>
>
> Diffs
> -----
>
> clients/src/main/java/org/apache/kafka/common/errors/NotEnoughReplicasAfterAppendException.java 75c80a97e43089cb3f924a38f86d67b5a8dd2b89
> core/src/main/scala/kafka/cluster/Partition.scala b230e9a1fb1a3161f4c9d164e4890a16eceb2ad4
>
> Diff: https://reviews.apache.org/r/29647/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Gwen Shapira
>
>
Re: Review Request 29647: Patch for KAFKA-1697
Posted by Ewen Cheslack-Postava <me...@ewencp.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29647/#review67044
-----------------------------------------------------------
It does look like this is all that needs to be removed. Only question I have is if the new behavior is what we want if the broker sees a request with requiredAcks > 1. Ideally this shouldn't happen, but someone could be using an earlier version of the client that accepted that setting and could independently update brokers to this newer version that doesn't support those values. (Alternatively, though unrelated to the original issue, a bad client library might allow an invalid value.) It looks like it'll now eventually fail (but slowly) since the value of requiredAcks isn't validated by the broker and checkEnoughReplicasReachOffset will never return true.
I think the broker should validate the requiredAcks value when the message is received, failing fast if the value isn't 0, 1, or negative. Then the DelayedProduce is only used if the value is negative and checkEnoughReplicasReachOffset shouldn't even take the requiredAcks parameter.
- Ewen Cheslack-Postava
On Jan. 7, 2015, 12:20 a.m., Gwen Shapira wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29647/
> -----------------------------------------------------------
>
> (Updated Jan. 7, 2015, 12:20 a.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1697
> https://issues.apache.org/jira/browse/KAFKA-1697
>
>
> Repository: kafka
>
>
> Description
> -------
>
> removed broker code for handling acks>1 and made NotEnoughReplicasAfterAppendException non-retriable
>
>
> Diffs
> -----
>
> clients/src/main/java/org/apache/kafka/common/errors/NotEnoughReplicasAfterAppendException.java 75c80a97e43089cb3f924a38f86d67b5a8dd2b89
> core/src/main/scala/kafka/cluster/Partition.scala b230e9a1fb1a3161f4c9d164e4890a16eceb2ad4
>
> 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, 11:34 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>
> >
> > Actually this is exactly what I meant. i.e., we only retry on retriableexceptions - since a subsequent retry may actually succeed. With the current patch, the producer will not retry on NotEnoughReplicasAfterAppendException. The side-effect in question here is duplicates. Duplicates can arise even for other errors (e.g., request timed out). So that side-effect is not compelling enough to warrant a change to make this non-retriable.
I see what you mean and it makes lots of sense.
Let me raise the question on the mailing list, to be doubly sure, because I recall a lot of discussion about this in KAFKA-1555.
If this can be retriable, we don't need two errors (NotEnoughReplicasAfterAppendException and NotEnoughReplicasException) and can simplify this a bit.
- Gwen
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29647/#review71874
-----------------------------------------------------------
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 Joel Koshy <jj...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29647/#review71874
-----------------------------------------------------------
clients/src/main/java/org/apache/kafka/common/errors/NotEnoughReplicasAfterAppendException.java
<https://reviews.apache.org/r/29647/#comment117765>
Actually this is exactly what I meant. i.e., we only retry on retriableexceptions - since a subsequent retry may actually succeed. With the current patch, the producer will not retry on NotEnoughReplicasAfterAppendException. The side-effect in question here is duplicates. Duplicates can arise even for other errors (e.g., request timed out). So that side-effect is not compelling enough to warrant a change to make this non-retriable.
- 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, 10:58 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>
> >
> > I think our interpretation of retriable is as follows (but we can discuss on the list if that needs to change): if the produce request hits an error, and there is absolutely no point in retrying then that is a non-retriable error. MessageSizeTooLarge is an example - since unless the producer changes the request to make the messages smaller there is no point in retrying.
This interpetation is not supported by code...
The new producer *will retry* on every retriable exception (see completeBatch() and canRetry() logic).
On the other hand, the client code using the producer is free to retry or not on any error.
We basically have 3 categories here:
1. Retrying may fix an issue and doesn't have any side-effects (for example, LeaderNotAvailable)
2. Retrying will never work, no point in trying (MessageTooLarge)
3. Retrying may fix the error but also has side-effects (NotEnoughReplicasAfterAppendException)
I'm pretty sure we don't want to automatically retry errors of category #3, but leave those up to the client.
And currently the only way to do it is not make them Retriable.
We can take this to the mailing list of you want to discuss farther.
- Gwen
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29647/#review71869
-----------------------------------------------------------
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 Joel Koshy <jj...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29647/#review71869
-----------------------------------------------------------
clients/src/main/java/org/apache/kafka/common/errors/NotEnoughReplicasAfterAppendException.java
<https://reviews.apache.org/r/29647/#comment117746>
I think our interpretation of retriable is as follows (but we can discuss on the list if that needs to change): if the produce request hits an error, and there is absolutely no point in retrying then that is a non-retriable error. MessageSizeTooLarge is an example - since unless the producer changes the request to make the messages smaller there is no point in retrying.
core/src/main/scala/kafka/server/KafkaApis.scala
<https://reviews.apache.org/r/29647/#comment117747>
That's true that the code already has special cases for requiredAcks == 0 - however, that code is executed in the callback from replica manager. So it's not a 100% clean separation.
- 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
>
>
Re: Review Request 29647: Patch for KAFKA-1697
Posted by Joel Koshy <jj...@gmail.com>.
-----------------------------------------------------------
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 Eric Olander <ol...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29647/#review68687
-----------------------------------------------------------
core/src/main/scala/kafka/cluster/Partition.scala
<https://reviews.apache.org/r/29647/#comment113038>
I realize this has absolutely nothing to do with the code under review, but ouch, this is a lot of unnecessary work. This boolean expression is all that is needed:
leaderReplicaIfLocal().isDefined && inSyncReplicas.size < assignedReplicas.size
core/src/main/scala/kafka/cluster/Partition.scala
<https://reviews.apache.org/r/29647/#comment113039>
Again, totally unrelated, but all this needs is:
Option(assignedReplicaMap.get(replicaId))
Option.apply() already does what this code is doing.
- Eric Olander
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, 10:10 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>
> >
> > Understood, but if someone uses required.acks -1 they would most likely be okay with duplicates and would rather have the data persisted with guarantees. What do you think?
My understanding is that if an exception is retriable, the producer will retry 3 times by default, while non-retriable exceptions need to be explicitly handled by the code using the producer (but they can still choose to retry).
I think that when trying can cause new issues (without necessarily resolving the problem), we should not retry by default but rather let the coder using the producer decide on their preferred behavior. Some of my customers are very sensitive for duplicates and may prefer not to retry immediatly on this issue but rather back off (or even log an error).
- Gwen
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29647/#review71857
-----------------------------------------------------------
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 Joel Koshy <jj...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29647/#review71857
-----------------------------------------------------------
clients/src/main/java/org/apache/kafka/common/errors/NotEnoughReplicasAfterAppendException.java
<https://reviews.apache.org/r/29647/#comment117734>
Understood, but if someone uses required.acks -1 they would most likely be okay with duplicates and would rather have the data persisted with guarantees. What do you think?
- 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. 13, 2015, 3:05 a.m., Joel Koshy wrote:
> > KAFKA-1948 changes crept in to the rb, but I can clean that up.
Thanks, man :)
I think I need lessons on how to do merges and use the patch-review tool...
- Gwen
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29647/#review72317
-----------------------------------------------------------
On Feb. 13, 2015, 2:57 a.m., Gwen Shapira wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29647/
> -----------------------------------------------------------
>
> (Updated Feb. 13, 2015, 2:57 a.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1697
> https://issues.apache.org/jira/browse/KAFKA-1697
>
>
> Repository: kafka
>
>
> Description
> -------
>
> added early handling of invalid number of acks to handler and a test
>
>
> merging with current trunk
>
>
> moved check for legal requiredAcks to append and fixed the tests accordingly
>
>
> changing exception back to retriable
>
>
> cleaning unused exceptions
>
>
> refactored appendToLog for clarity
>
>
> KAFKA-1948; Fix ConsumerTest.testPartitionReassignmentCallback handling issue; reviewed by Gwen Shapira
>
>
> Merge branch 'trunk' of http://git-wip-us.apache.org/repos/asf/kafka into KAFKA-1697
>
>
> improved readability of append rules
>
>
> 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 a6107b818947d6d6818c85cdffcb2b13f69a55c0
> clients/src/main/java/org/apache/kafka/common/protocol/Errors.java a8deac4ce5149129d0a6f44c0526af9d55649a36
> core/src/main/scala/kafka/cluster/Partition.scala e6ad8be5e33b6fb31c078ad78f8de709869ddc04
> core/src/main/scala/kafka/controller/KafkaController.scala 66df6d2fbdbdd556da6bea0df84f93e0472c8fbf
> core/src/main/scala/kafka/server/KafkaApis.scala 6ee7d8819a9ef923f3a65c865a0a3d8ded8845f0
> core/src/main/scala/kafka/server/ReplicaManager.scala fb948b9ab28c516e81dab14dcbe211dcd99842b6
> core/src/test/scala/integration/kafka/api/ConsumerTest.scala 798f035df52e405176f558806584ce25e8c392ac
> core/src/test/scala/unit/kafka/api/RequestResponseSerializationTest.scala a1f72f8c2042ff2a43af503b2e06f84706dad9db
> core/src/test/scala/unit/kafka/server/ReplicaManagerTest.scala faa907131ed0aa94a7eacb78c1ffb576062be87a
>
> Diff: https://reviews.apache.org/r/29647/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Gwen Shapira
>
>
Re: Review Request 29647: Patch for KAFKA-1697
Posted by Joel Koshy <jj...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29647/#review72317
-----------------------------------------------------------
Ship it!
KAFKA-1948 changes crept in to the rb, but I can clean that up.
- Joel Koshy
On Feb. 13, 2015, 2:57 a.m., Gwen Shapira wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29647/
> -----------------------------------------------------------
>
> (Updated Feb. 13, 2015, 2:57 a.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1697
> https://issues.apache.org/jira/browse/KAFKA-1697
>
>
> Repository: kafka
>
>
> Description
> -------
>
> added early handling of invalid number of acks to handler and a test
>
>
> merging with current trunk
>
>
> moved check for legal requiredAcks to append and fixed the tests accordingly
>
>
> changing exception back to retriable
>
>
> cleaning unused exceptions
>
>
> refactored appendToLog for clarity
>
>
> KAFKA-1948; Fix ConsumerTest.testPartitionReassignmentCallback handling issue; reviewed by Gwen Shapira
>
>
> Merge branch 'trunk' of http://git-wip-us.apache.org/repos/asf/kafka into KAFKA-1697
>
>
> improved readability of append rules
>
>
> 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 a6107b818947d6d6818c85cdffcb2b13f69a55c0
> clients/src/main/java/org/apache/kafka/common/protocol/Errors.java a8deac4ce5149129d0a6f44c0526af9d55649a36
> core/src/main/scala/kafka/cluster/Partition.scala e6ad8be5e33b6fb31c078ad78f8de709869ddc04
> core/src/main/scala/kafka/controller/KafkaController.scala 66df6d2fbdbdd556da6bea0df84f93e0472c8fbf
> core/src/main/scala/kafka/server/KafkaApis.scala 6ee7d8819a9ef923f3a65c865a0a3d8ded8845f0
> core/src/main/scala/kafka/server/ReplicaManager.scala fb948b9ab28c516e81dab14dcbe211dcd99842b6
> core/src/test/scala/integration/kafka/api/ConsumerTest.scala 798f035df52e405176f558806584ce25e8c392ac
> core/src/test/scala/unit/kafka/api/RequestResponseSerializationTest.scala a1f72f8c2042ff2a43af503b2e06f84706dad9db
> core/src/test/scala/unit/kafka/server/ReplicaManagerTest.scala faa907131ed0aa94a7eacb78c1ffb576062be87a
>
> Diff: https://reviews.apache.org/r/29647/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Gwen Shapira
>
>
Re: Review Request 29647: Patch for KAFKA-1697
Posted by Jun Rao <ju...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29647/#review74592
-----------------------------------------------------------
Sorry for the late review. Have one comment below.
core/src/main/scala/kafka/cluster/Partition.scala
<https://reviews.apache.org/r/29647/#comment121213>
Should we just remove requiredAcks completely since checkEnoughReplicasReachOffset() will only be called when requiredAcks is -1?
- Jun Rao
On Feb. 13, 2015, 2:57 a.m., Gwen Shapira wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29647/
> -----------------------------------------------------------
>
> (Updated Feb. 13, 2015, 2:57 a.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1697
> https://issues.apache.org/jira/browse/KAFKA-1697
>
>
> Repository: kafka
>
>
> Description
> -------
>
> added early handling of invalid number of acks to handler and a test
>
>
> merging with current trunk
>
>
> moved check for legal requiredAcks to append and fixed the tests accordingly
>
>
> changing exception back to retriable
>
>
> cleaning unused exceptions
>
>
> refactored appendToLog for clarity
>
>
> KAFKA-1948; Fix ConsumerTest.testPartitionReassignmentCallback handling issue; reviewed by Gwen Shapira
>
>
> Merge branch 'trunk' of http://git-wip-us.apache.org/repos/asf/kafka into KAFKA-1697
>
>
> improved readability of append rules
>
>
> 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 a6107b818947d6d6818c85cdffcb2b13f69a55c0
> clients/src/main/java/org/apache/kafka/common/protocol/Errors.java a8deac4ce5149129d0a6f44c0526af9d55649a36
> core/src/main/scala/kafka/cluster/Partition.scala e6ad8be5e33b6fb31c078ad78f8de709869ddc04
> core/src/main/scala/kafka/controller/KafkaController.scala 66df6d2fbdbdd556da6bea0df84f93e0472c8fbf
> core/src/main/scala/kafka/server/KafkaApis.scala 6ee7d8819a9ef923f3a65c865a0a3d8ded8845f0
> core/src/main/scala/kafka/server/ReplicaManager.scala fb948b9ab28c516e81dab14dcbe211dcd99842b6
> core/src/test/scala/integration/kafka/api/ConsumerTest.scala 798f035df52e405176f558806584ce25e8c392ac
> core/src/test/scala/unit/kafka/api/RequestResponseSerializationTest.scala a1f72f8c2042ff2a43af503b2e06f84706dad9db
> core/src/test/scala/unit/kafka/server/ReplicaManagerTest.scala faa907131ed0aa94a7eacb78c1ffb576062be87a
>
> 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. 27, 2015, 11:18 p.m., Joel Koshy wrote:
> > core/src/main/scala/kafka/cluster/Partition.scala, line 289
> > <https://reviews.apache.org/r/29647/diff/7/?file=862811#file862811line289>
> >
> > good point
Yep, good catch. Created KAFKA-1992 to fix this.
- Gwen
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29647/#review74623
-----------------------------------------------------------
On Feb. 13, 2015, 2:57 a.m., Gwen Shapira wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29647/
> -----------------------------------------------------------
>
> (Updated Feb. 13, 2015, 2:57 a.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1697
> https://issues.apache.org/jira/browse/KAFKA-1697
>
>
> Repository: kafka
>
>
> Description
> -------
>
> added early handling of invalid number of acks to handler and a test
>
>
> merging with current trunk
>
>
> moved check for legal requiredAcks to append and fixed the tests accordingly
>
>
> changing exception back to retriable
>
>
> cleaning unused exceptions
>
>
> refactored appendToLog for clarity
>
>
> KAFKA-1948; Fix ConsumerTest.testPartitionReassignmentCallback handling issue; reviewed by Gwen Shapira
>
>
> Merge branch 'trunk' of http://git-wip-us.apache.org/repos/asf/kafka into KAFKA-1697
>
>
> improved readability of append rules
>
>
> 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 a6107b818947d6d6818c85cdffcb2b13f69a55c0
> clients/src/main/java/org/apache/kafka/common/protocol/Errors.java a8deac4ce5149129d0a6f44c0526af9d55649a36
> core/src/main/scala/kafka/cluster/Partition.scala e6ad8be5e33b6fb31c078ad78f8de709869ddc04
> core/src/main/scala/kafka/controller/KafkaController.scala 66df6d2fbdbdd556da6bea0df84f93e0472c8fbf
> core/src/main/scala/kafka/server/KafkaApis.scala 6ee7d8819a9ef923f3a65c865a0a3d8ded8845f0
> core/src/main/scala/kafka/server/ReplicaManager.scala fb948b9ab28c516e81dab14dcbe211dcd99842b6
> core/src/test/scala/integration/kafka/api/ConsumerTest.scala 798f035df52e405176f558806584ce25e8c392ac
> core/src/test/scala/unit/kafka/api/RequestResponseSerializationTest.scala a1f72f8c2042ff2a43af503b2e06f84706dad9db
> core/src/test/scala/unit/kafka/server/ReplicaManagerTest.scala faa907131ed0aa94a7eacb78c1ffb576062be87a
>
> Diff: https://reviews.apache.org/r/29647/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Gwen Shapira
>
>
Re: Review Request 29647: Patch for KAFKA-1697
Posted by Joel Koshy <jj...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29647/#review74623
-----------------------------------------------------------
core/src/main/scala/kafka/cluster/Partition.scala
<https://reviews.apache.org/r/29647/#comment121245>
good point
- Joel Koshy
On Feb. 13, 2015, 2:57 a.m., Gwen Shapira wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29647/
> -----------------------------------------------------------
>
> (Updated Feb. 13, 2015, 2:57 a.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1697
> https://issues.apache.org/jira/browse/KAFKA-1697
>
>
> Repository: kafka
>
>
> Description
> -------
>
> added early handling of invalid number of acks to handler and a test
>
>
> merging with current trunk
>
>
> moved check for legal requiredAcks to append and fixed the tests accordingly
>
>
> changing exception back to retriable
>
>
> cleaning unused exceptions
>
>
> refactored appendToLog for clarity
>
>
> KAFKA-1948; Fix ConsumerTest.testPartitionReassignmentCallback handling issue; reviewed by Gwen Shapira
>
>
> Merge branch 'trunk' of http://git-wip-us.apache.org/repos/asf/kafka into KAFKA-1697
>
>
> improved readability of append rules
>
>
> 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 a6107b818947d6d6818c85cdffcb2b13f69a55c0
> clients/src/main/java/org/apache/kafka/common/protocol/Errors.java a8deac4ce5149129d0a6f44c0526af9d55649a36
> core/src/main/scala/kafka/cluster/Partition.scala e6ad8be5e33b6fb31c078ad78f8de709869ddc04
> core/src/main/scala/kafka/controller/KafkaController.scala 66df6d2fbdbdd556da6bea0df84f93e0472c8fbf
> core/src/main/scala/kafka/server/KafkaApis.scala 6ee7d8819a9ef923f3a65c865a0a3d8ded8845f0
> core/src/main/scala/kafka/server/ReplicaManager.scala fb948b9ab28c516e81dab14dcbe211dcd99842b6
> core/src/test/scala/integration/kafka/api/ConsumerTest.scala 798f035df52e405176f558806584ce25e8c392ac
> core/src/test/scala/unit/kafka/api/RequestResponseSerializationTest.scala a1f72f8c2042ff2a43af503b2e06f84706dad9db
> core/src/test/scala/unit/kafka/server/ReplicaManagerTest.scala faa907131ed0aa94a7eacb78c1ffb576062be87a
>
> 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>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29647/
-----------------------------------------------------------
(Updated Feb. 13, 2015, 2:57 a.m.)
Review request for kafka.
Bugs: KAFKA-1697
https://issues.apache.org/jira/browse/KAFKA-1697
Repository: kafka
Description (updated)
-------
added early handling of invalid number of acks to handler and a test
merging with current trunk
moved check for legal requiredAcks to append and fixed the tests accordingly
changing exception back to retriable
cleaning unused exceptions
refactored appendToLog for clarity
KAFKA-1948; Fix ConsumerTest.testPartitionReassignmentCallback handling issue; reviewed by Gwen Shapira
Merge branch 'trunk' of http://git-wip-us.apache.org/repos/asf/kafka into KAFKA-1697
improved readability of append rules
Diffs (updated)
-----
clients/src/main/java/org/apache/kafka/common/errors/InvalidRequiredAcksException.java PRE-CREATION
clients/src/main/java/org/apache/kafka/common/errors/NotEnoughReplicasAfterAppendException.java a6107b818947d6d6818c85cdffcb2b13f69a55c0
clients/src/main/java/org/apache/kafka/common/protocol/Errors.java a8deac4ce5149129d0a6f44c0526af9d55649a36
core/src/main/scala/kafka/cluster/Partition.scala e6ad8be5e33b6fb31c078ad78f8de709869ddc04
core/src/main/scala/kafka/controller/KafkaController.scala 66df6d2fbdbdd556da6bea0df84f93e0472c8fbf
core/src/main/scala/kafka/server/KafkaApis.scala 6ee7d8819a9ef923f3a65c865a0a3d8ded8845f0
core/src/main/scala/kafka/server/ReplicaManager.scala fb948b9ab28c516e81dab14dcbe211dcd99842b6
core/src/test/scala/integration/kafka/api/ConsumerTest.scala 798f035df52e405176f558806584ce25e8c392ac
core/src/test/scala/unit/kafka/api/RequestResponseSerializationTest.scala a1f72f8c2042ff2a43af503b2e06f84706dad9db
core/src/test/scala/unit/kafka/server/ReplicaManagerTest.scala faa907131ed0aa94a7eacb78c1ffb576062be87a
Diff: https://reviews.apache.org/r/29647/diff/
Testing
-------
Thanks,
Gwen Shapira
Re: Review Request 29647: Patch for KAFKA-1697
Posted by Joel Koshy <jj...@gmail.com>.
> On Feb. 12, 2015, 2:31 p.m., Joel Koshy wrote:
> > core/src/main/scala/kafka/server/ReplicaManager.scala, line 271
> > <https://reviews.apache.org/r/29647/diff/6/?file=861625#file861625line271>
> >
> > This is good, but maybe call this canRespondNow ? Since before replication won't apply if there are no errors in the partition appends or if there is no data.
In fact flipping the condition and calling the helper function delayedRequestRequired may be even clearer
- Joel
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29647/#review72156
-----------------------------------------------------------
On Feb. 12, 2015, 7:14 a.m., Gwen Shapira wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29647/
> -----------------------------------------------------------
>
> (Updated Feb. 12, 2015, 7:14 a.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1697
> https://issues.apache.org/jira/browse/KAFKA-1697
>
>
> Repository: kafka
>
>
> Description
> -------
>
> added early handling of invalid number of acks to handler and a test
>
>
> merging with current trunk
>
>
> moved check for legal requiredAcks to append and fixed the tests accordingly
>
>
> changing exception back to retriable
>
>
> cleaning unused exceptions
>
>
> refactored appendToLog for clarity
>
>
> 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 a6107b818947d6d6818c85cdffcb2b13f69a55c0
> clients/src/main/java/org/apache/kafka/common/protocol/Errors.java a8deac4ce5149129d0a6f44c0526af9d55649a36
> core/src/main/scala/kafka/cluster/Partition.scala e6ad8be5e33b6fb31c078ad78f8de709869ddc04
> core/src/main/scala/kafka/server/KafkaApis.scala 6ee7d8819a9ef923f3a65c865a0a3d8ded8845f0
> core/src/main/scala/kafka/server/ReplicaManager.scala fb948b9ab28c516e81dab14dcbe211dcd99842b6
> core/src/test/scala/unit/kafka/api/RequestResponseSerializationTest.scala a1f72f8c2042ff2a43af503b2e06f84706dad9db
> core/src/test/scala/unit/kafka/server/ReplicaManagerTest.scala faa907131ed0aa94a7eacb78c1ffb576062be87a
>
> 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. 12, 2015, 2:31 p.m., Joel Koshy wrote:
> > core/src/test/scala/unit/kafka/api/RequestResponseSerializationTest.scala, line 62
> > <https://reviews.apache.org/r/29647/diff/6/?file=861626#file861626line62>
> >
> > Is this change necessary?
>
> Gwen Shapira wrote:
> Nope, but I need to change the test to use produceRequest from TestUtils and not the API test.
> It snack in when I moved the test code around.
>
> Let me fix this.
Actually, the methods from SerializationTestUtils are super useful, and its a bit of a bummer that the are private and part of RequestResponseSerializationTest.
Do you mind leaving this as is and I'll open a separate JIRA to refactor those test utils into something reusable?
- Gwen
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29647/#review72156
-----------------------------------------------------------
On Feb. 12, 2015, 7:14 a.m., Gwen Shapira wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29647/
> -----------------------------------------------------------
>
> (Updated Feb. 12, 2015, 7:14 a.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1697
> https://issues.apache.org/jira/browse/KAFKA-1697
>
>
> Repository: kafka
>
>
> Description
> -------
>
> added early handling of invalid number of acks to handler and a test
>
>
> merging with current trunk
>
>
> moved check for legal requiredAcks to append and fixed the tests accordingly
>
>
> changing exception back to retriable
>
>
> cleaning unused exceptions
>
>
> refactored appendToLog for clarity
>
>
> 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 a6107b818947d6d6818c85cdffcb2b13f69a55c0
> clients/src/main/java/org/apache/kafka/common/protocol/Errors.java a8deac4ce5149129d0a6f44c0526af9d55649a36
> core/src/main/scala/kafka/cluster/Partition.scala e6ad8be5e33b6fb31c078ad78f8de709869ddc04
> core/src/main/scala/kafka/server/KafkaApis.scala 6ee7d8819a9ef923f3a65c865a0a3d8ded8845f0
> core/src/main/scala/kafka/server/ReplicaManager.scala fb948b9ab28c516e81dab14dcbe211dcd99842b6
> core/src/test/scala/unit/kafka/api/RequestResponseSerializationTest.scala a1f72f8c2042ff2a43af503b2e06f84706dad9db
> core/src/test/scala/unit/kafka/server/ReplicaManagerTest.scala faa907131ed0aa94a7eacb78c1ffb576062be87a
>
> 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. 12, 2015, 2:31 p.m., Joel Koshy wrote:
> > core/src/main/scala/kafka/server/ReplicaManager.scala, line 271
> > <https://reviews.apache.org/r/29647/diff/6/?file=861625#file861625line271>
> >
> > This is good, but maybe call this canRespondNow ? Since before replication won't apply if there are no errors in the partition appends or if there is no data.
>
> Joel Koshy wrote:
> In fact flipping the condition and calling the helper function delayedRequestRequired may be even clearer
good point. Fixing this.
> On Feb. 12, 2015, 2:31 p.m., Joel Koshy wrote:
> > core/src/test/scala/unit/kafka/api/RequestResponseSerializationTest.scala, line 62
> > <https://reviews.apache.org/r/29647/diff/6/?file=861626#file861626line62>
> >
> > Is this change necessary?
Nope, but I need to change the test to use produceRequest from TestUtils and not the API test.
It snack in when I moved the test code around.
Let me fix this.
- Gwen
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29647/#review72156
-----------------------------------------------------------
On Feb. 12, 2015, 7:14 a.m., Gwen Shapira wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29647/
> -----------------------------------------------------------
>
> (Updated Feb. 12, 2015, 7:14 a.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1697
> https://issues.apache.org/jira/browse/KAFKA-1697
>
>
> Repository: kafka
>
>
> Description
> -------
>
> added early handling of invalid number of acks to handler and a test
>
>
> merging with current trunk
>
>
> moved check for legal requiredAcks to append and fixed the tests accordingly
>
>
> changing exception back to retriable
>
>
> cleaning unused exceptions
>
>
> refactored appendToLog for clarity
>
>
> 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 a6107b818947d6d6818c85cdffcb2b13f69a55c0
> clients/src/main/java/org/apache/kafka/common/protocol/Errors.java a8deac4ce5149129d0a6f44c0526af9d55649a36
> core/src/main/scala/kafka/cluster/Partition.scala e6ad8be5e33b6fb31c078ad78f8de709869ddc04
> core/src/main/scala/kafka/server/KafkaApis.scala 6ee7d8819a9ef923f3a65c865a0a3d8ded8845f0
> core/src/main/scala/kafka/server/ReplicaManager.scala fb948b9ab28c516e81dab14dcbe211dcd99842b6
> core/src/test/scala/unit/kafka/api/RequestResponseSerializationTest.scala a1f72f8c2042ff2a43af503b2e06f84706dad9db
> core/src/test/scala/unit/kafka/server/ReplicaManagerTest.scala faa907131ed0aa94a7eacb78c1ffb576062be87a
>
> Diff: https://reviews.apache.org/r/29647/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Gwen Shapira
>
>
Re: Review Request 29647: Patch for KAFKA-1697
Posted by Joel Koshy <jj...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29647/#review72156
-----------------------------------------------------------
Ship it!
Thanks Gwen - a few minor comments that I can fix up - can you comment on these?
core/src/main/scala/kafka/server/ReplicaManager.scala
<https://reviews.apache.org/r/29647/#comment118159>
This is good, but maybe call this canRespondNow ? Since before replication won't apply if there are no errors in the partition appends or if there is no data.
core/src/test/scala/unit/kafka/api/RequestResponseSerializationTest.scala
<https://reviews.apache.org/r/29647/#comment118160>
Is this change necessary?
- Joel Koshy
On Feb. 12, 2015, 7:14 a.m., Gwen Shapira wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29647/
> -----------------------------------------------------------
>
> (Updated Feb. 12, 2015, 7:14 a.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1697
> https://issues.apache.org/jira/browse/KAFKA-1697
>
>
> Repository: kafka
>
>
> Description
> -------
>
> added early handling of invalid number of acks to handler and a test
>
>
> merging with current trunk
>
>
> moved check for legal requiredAcks to append and fixed the tests accordingly
>
>
> changing exception back to retriable
>
>
> cleaning unused exceptions
>
>
> refactored appendToLog for clarity
>
>
> 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 a6107b818947d6d6818c85cdffcb2b13f69a55c0
> clients/src/main/java/org/apache/kafka/common/protocol/Errors.java a8deac4ce5149129d0a6f44c0526af9d55649a36
> core/src/main/scala/kafka/cluster/Partition.scala e6ad8be5e33b6fb31c078ad78f8de709869ddc04
> core/src/main/scala/kafka/server/KafkaApis.scala 6ee7d8819a9ef923f3a65c865a0a3d8ded8845f0
> core/src/main/scala/kafka/server/ReplicaManager.scala fb948b9ab28c516e81dab14dcbe211dcd99842b6
> core/src/test/scala/unit/kafka/api/RequestResponseSerializationTest.scala a1f72f8c2042ff2a43af503b2e06f84706dad9db
> core/src/test/scala/unit/kafka/server/ReplicaManagerTest.scala faa907131ed0aa94a7eacb78c1ffb576062be87a
>
> 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>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29647/
-----------------------------------------------------------
(Updated Feb. 12, 2015, 7:14 a.m.)
Review request for kafka.
Bugs: KAFKA-1697
https://issues.apache.org/jira/browse/KAFKA-1697
Repository: kafka
Description (updated)
-------
added early handling of invalid number of acks to handler and a test
merging with current trunk
moved check for legal requiredAcks to append and fixed the tests accordingly
changing exception back to retriable
cleaning unused exceptions
refactored appendToLog for clarity
Diffs (updated)
-----
clients/src/main/java/org/apache/kafka/common/errors/InvalidRequiredAcksException.java PRE-CREATION
clients/src/main/java/org/apache/kafka/common/errors/NotEnoughReplicasAfterAppendException.java a6107b818947d6d6818c85cdffcb2b13f69a55c0
clients/src/main/java/org/apache/kafka/common/protocol/Errors.java a8deac4ce5149129d0a6f44c0526af9d55649a36
core/src/main/scala/kafka/cluster/Partition.scala e6ad8be5e33b6fb31c078ad78f8de709869ddc04
core/src/main/scala/kafka/server/KafkaApis.scala 6ee7d8819a9ef923f3a65c865a0a3d8ded8845f0
core/src/main/scala/kafka/server/ReplicaManager.scala fb948b9ab28c516e81dab14dcbe211dcd99842b6
core/src/test/scala/unit/kafka/api/RequestResponseSerializationTest.scala a1f72f8c2042ff2a43af503b2e06f84706dad9db
core/src/test/scala/unit/kafka/server/ReplicaManagerTest.scala faa907131ed0aa94a7eacb78c1ffb576062be87a
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. 12, 2015, 4:20 a.m., Joe Stein wrote:
> > core/src/main/scala/kafka/server/ReplicaManager.scala, line 307
> > <https://reviews.apache.org/r/29647/diff/5/?file=861501#file861501line307>
> >
> > Could we change this to a match case?
> > requiredAcks match {
> > case 0 => {}
> > case 1 => {}
> > case -1 => {}
> > case _ => {}
> > }
> > make it more concise to read through and scalish
unfortunately, I couldn't use a switch on requiredAcks. There's logic for whether we need to wait for replication based on both requiredAcks and other factors, and splitting this logic to multiple switches / conditions makes things less readable.
I do agree that it was challenging to read, so I did some refactoring. I hope the new version is clearer, even if not very scalish.
- Gwen
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29647/#review72107
-----------------------------------------------------------
On Feb. 12, 2015, 7:14 a.m., Gwen Shapira wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29647/
> -----------------------------------------------------------
>
> (Updated Feb. 12, 2015, 7:14 a.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1697
> https://issues.apache.org/jira/browse/KAFKA-1697
>
>
> Repository: kafka
>
>
> Description
> -------
>
> added early handling of invalid number of acks to handler and a test
>
>
> merging with current trunk
>
>
> moved check for legal requiredAcks to append and fixed the tests accordingly
>
>
> changing exception back to retriable
>
>
> cleaning unused exceptions
>
>
> refactored appendToLog for clarity
>
>
> 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 a6107b818947d6d6818c85cdffcb2b13f69a55c0
> clients/src/main/java/org/apache/kafka/common/protocol/Errors.java a8deac4ce5149129d0a6f44c0526af9d55649a36
> core/src/main/scala/kafka/cluster/Partition.scala e6ad8be5e33b6fb31c078ad78f8de709869ddc04
> core/src/main/scala/kafka/server/KafkaApis.scala 6ee7d8819a9ef923f3a65c865a0a3d8ded8845f0
> core/src/main/scala/kafka/server/ReplicaManager.scala fb948b9ab28c516e81dab14dcbe211dcd99842b6
> core/src/test/scala/unit/kafka/api/RequestResponseSerializationTest.scala a1f72f8c2042ff2a43af503b2e06f84706dad9db
> core/src/test/scala/unit/kafka/server/ReplicaManagerTest.scala faa907131ed0aa94a7eacb78c1ffb576062be87a
>
> Diff: https://reviews.apache.org/r/29647/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Gwen Shapira
>
>
Re: Review Request 29647: Patch for KAFKA-1697
Posted by Joe Stein <cr...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29647/#review72107
-----------------------------------------------------------
core/src/main/scala/kafka/server/ReplicaManager.scala
<https://reviews.apache.org/r/29647/#comment118051>
Could we change this to a match case?
requiredAcks match {
case 0 => {}
case 1 => {}
case -1 => {}
case _ => {}
}
make it more concise to read through and scalish
- Joe Stein
On Feb. 12, 2015, 2:47 a.m., Gwen Shapira wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29647/
> -----------------------------------------------------------
>
> (Updated Feb. 12, 2015, 2:47 a.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1697
> https://issues.apache.org/jira/browse/KAFKA-1697
>
>
> Repository: kafka
>
>
> Description
> -------
>
> added early handling of invalid number of acks to handler and a test
>
>
> merging with current trunk
>
>
> moved check for legal requiredAcks to append and fixed the tests accordingly
>
>
> changing exception back to retriable
>
>
> cleaning unused exceptions
>
>
> 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 a6107b818947d6d6818c85cdffcb2b13f69a55c0
> clients/src/main/java/org/apache/kafka/common/protocol/Errors.java a8deac4ce5149129d0a6f44c0526af9d55649a36
> core/src/main/scala/kafka/cluster/Partition.scala e6ad8be5e33b6fb31c078ad78f8de709869ddc04
> core/src/main/scala/kafka/server/KafkaApis.scala 6ee7d8819a9ef923f3a65c865a0a3d8ded8845f0
> core/src/main/scala/kafka/server/ReplicaManager.scala fb948b9ab28c516e81dab14dcbe211dcd99842b6
> core/src/test/scala/unit/kafka/api/RequestResponseSerializationTest.scala a1f72f8c2042ff2a43af503b2e06f84706dad9db
> core/src/test/scala/unit/kafka/server/ReplicaManagerTest.scala faa907131ed0aa94a7eacb78c1ffb576062be87a
>
> 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>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29647/
-----------------------------------------------------------
(Updated Feb. 12, 2015, 2:47 a.m.)
Review request for kafka.
Bugs: KAFKA-1697
https://issues.apache.org/jira/browse/KAFKA-1697
Repository: kafka
Description (updated)
-------
added early handling of invalid number of acks to handler and a test
merging with current trunk
moved check for legal requiredAcks to append and fixed the tests accordingly
changing exception back to retriable
cleaning unused exceptions
Diffs (updated)
-----
clients/src/main/java/org/apache/kafka/common/errors/InvalidRequiredAcksException.java PRE-CREATION
clients/src/main/java/org/apache/kafka/common/errors/NotEnoughReplicasAfterAppendException.java a6107b818947d6d6818c85cdffcb2b13f69a55c0
clients/src/main/java/org/apache/kafka/common/protocol/Errors.java a8deac4ce5149129d0a6f44c0526af9d55649a36
core/src/main/scala/kafka/cluster/Partition.scala e6ad8be5e33b6fb31c078ad78f8de709869ddc04
core/src/main/scala/kafka/server/KafkaApis.scala 6ee7d8819a9ef923f3a65c865a0a3d8ded8845f0
core/src/main/scala/kafka/server/ReplicaManager.scala fb948b9ab28c516e81dab14dcbe211dcd99842b6
core/src/test/scala/unit/kafka/api/RequestResponseSerializationTest.scala a1f72f8c2042ff2a43af503b2e06f84706dad9db
core/src/test/scala/unit/kafka/server/ReplicaManagerTest.scala faa907131ed0aa94a7eacb78c1ffb576062be87a
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>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29647/
-----------------------------------------------------------
(Updated Feb. 12, 2015, 2:45 a.m.)
Review request for kafka.
Bugs: KAFKA-1697
https://issues.apache.org/jira/browse/KAFKA-1697
Repository: kafka
Description (updated)
-------
added early handling of invalid number of acks to handler and a test
merging with current trunk
moved check for legal requiredAcks to append and fixed the tests accordingly
changing exception back to retriable
Diffs (updated)
-----
clients/src/main/java/org/apache/kafka/common/errors/InvalidRequiredAcksException.java PRE-CREATION
clients/src/main/java/org/apache/kafka/common/errors/NotEnoughReplicasAfterAppendException.java a6107b818947d6d6818c85cdffcb2b13f69a55c0
clients/src/main/java/org/apache/kafka/common/protocol/Errors.java a8deac4ce5149129d0a6f44c0526af9d55649a36
core/src/main/scala/kafka/cluster/Partition.scala e6ad8be5e33b6fb31c078ad78f8de709869ddc04
core/src/main/scala/kafka/server/KafkaApis.scala 6ee7d8819a9ef923f3a65c865a0a3d8ded8845f0
core/src/main/scala/kafka/server/ReplicaManager.scala fb948b9ab28c516e81dab14dcbe211dcd99842b6
core/src/test/scala/unit/kafka/api/RequestResponseSerializationTest.scala a1f72f8c2042ff2a43af503b2e06f84706dad9db
core/src/test/scala/unit/kafka/server/ReplicaManagerTest.scala faa907131ed0aa94a7eacb78c1ffb576062be87a
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. 11, 2015, 1:48 p.m., Joel Koshy wrote:
> > clients/src/main/java/org/apache/kafka/common/errors/NotEnoughReplicasAfterAppendException.java, line 23
> > <https://reviews.apache.org/r/29647/diff/3/?file=860600#file860600line23>
> >
> > Pending discussion.
>
> Gwen Shapira wrote:
> changing back to retriable, per discussion in mailing list.
> I'm leaving this as a separate exception and error code, in case client developers want to do something with the extra information.
Actually, since we are here, I'll remove the error code from kafka.common and use the o.a.k.common.errors everywhere.
We are transitioning there anyway.
- Gwen
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29647/#review71960
-----------------------------------------------------------
On Feb. 11, 2015, 1:06 a.m., Gwen Shapira wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29647/
> -----------------------------------------------------------
>
> (Updated Feb. 11, 2015, 1:06 a.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1697
> https://issues.apache.org/jira/browse/KAFKA-1697
>
>
> Repository: kafka
>
>
> Description
> -------
>
> added early handling of invalid number of acks to handler and a test
>
>
> merging with current trunk
>
>
> moved check for legal requiredAcks to append and fixed the tests accordingly
>
>
> 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 a6107b818947d6d6818c85cdffcb2b13f69a55c0
> clients/src/main/java/org/apache/kafka/common/protocol/Errors.java a8deac4ce5149129d0a6f44c0526af9d55649a36
> core/src/main/scala/kafka/cluster/Partition.scala e6ad8be5e33b6fb31c078ad78f8de709869ddc04
> core/src/main/scala/kafka/server/KafkaApis.scala 6ee7d8819a9ef923f3a65c865a0a3d8ded8845f0
> core/src/main/scala/kafka/server/ReplicaManager.scala fb948b9ab28c516e81dab14dcbe211dcd99842b6
> core/src/test/scala/unit/kafka/server/ReplicaManagerTest.scala faa907131ed0aa94a7eacb78c1ffb576062be87a
>
> 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. 11, 2015, 1:48 p.m., Joel Koshy wrote:
> > clients/src/main/java/org/apache/kafka/common/errors/NotEnoughReplicasAfterAppendException.java, line 23
> > <https://reviews.apache.org/r/29647/diff/3/?file=860600#file860600line23>
> >
> > Pending discussion.
>
> Gwen Shapira wrote:
> changing back to retriable, per discussion in mailing list.
> I'm leaving this as a separate exception and error code, in case client developers want to do something with the extra information.
>
> Gwen Shapira wrote:
> Actually, since we are here, I'll remove the error code from kafka.common and use the o.a.k.common.errors everywhere.
> We are transitioning there anyway.
Never mind on the last comment. I have unit tests for NotEnoughReplicas that use the scala producer and that requires the kafka.common error.
I'll let Jeff figure out how to disentangle the Scala Producer from the existing scala error codes. Its way beyond scope here :)
- Gwen
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29647/#review71960
-----------------------------------------------------------
On Feb. 11, 2015, 1:06 a.m., Gwen Shapira wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29647/
> -----------------------------------------------------------
>
> (Updated Feb. 11, 2015, 1:06 a.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1697
> https://issues.apache.org/jira/browse/KAFKA-1697
>
>
> Repository: kafka
>
>
> Description
> -------
>
> added early handling of invalid number of acks to handler and a test
>
>
> merging with current trunk
>
>
> moved check for legal requiredAcks to append and fixed the tests accordingly
>
>
> 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 a6107b818947d6d6818c85cdffcb2b13f69a55c0
> clients/src/main/java/org/apache/kafka/common/protocol/Errors.java a8deac4ce5149129d0a6f44c0526af9d55649a36
> core/src/main/scala/kafka/cluster/Partition.scala e6ad8be5e33b6fb31c078ad78f8de709869ddc04
> core/src/main/scala/kafka/server/KafkaApis.scala 6ee7d8819a9ef923f3a65c865a0a3d8ded8845f0
> core/src/main/scala/kafka/server/ReplicaManager.scala fb948b9ab28c516e81dab14dcbe211dcd99842b6
> core/src/test/scala/unit/kafka/server/ReplicaManagerTest.scala faa907131ed0aa94a7eacb78c1ffb576062be87a
>
> 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. 11, 2015, 1:48 p.m., Joel Koshy wrote:
> > core/src/test/scala/unit/kafka/server/ReplicaManagerTest.scala, line 96
> > <https://reviews.apache.org/r/29647/diff/3/?file=860605#file860605line96>
> >
> > Do we need this here?
IMO, its a good idea to have this in any test that starts new threads - to verify that we close them.
We have this in a bunch of places.
> On Feb. 11, 2015, 1:48 p.m., Joel Koshy wrote:
> > core/src/test/scala/unit/kafka/server/ReplicaManagerTest.scala, line 83
> > <https://reviews.apache.org/r/29647/diff/3/?file=860605#file860605line83>
> >
> > Can we just do the assert here instead of throwing an exception?
> >
> > i.e., `assertEquals(responseStatus.values.size, responseStatus.values.count(_.error == INVALID_REQUIRED_ACKS))`
> >
> > Either way is fine. Whichever is clearer although the above may be more concise if it works.
good idea.
> On Feb. 11, 2015, 1:48 p.m., Joel Koshy wrote:
> > clients/src/main/java/org/apache/kafka/common/errors/NotEnoughReplicasAfterAppendException.java, line 23
> > <https://reviews.apache.org/r/29647/diff/3/?file=860600#file860600line23>
> >
> > Pending discussion.
changing back to retriable, per discussion in mailing list.
I'm leaving this as a separate exception and error code, in case client developers want to do something with the extra information.
- Gwen
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29647/#review71960
-----------------------------------------------------------
On Feb. 11, 2015, 1:06 a.m., Gwen Shapira wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29647/
> -----------------------------------------------------------
>
> (Updated Feb. 11, 2015, 1:06 a.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1697
> https://issues.apache.org/jira/browse/KAFKA-1697
>
>
> Repository: kafka
>
>
> Description
> -------
>
> added early handling of invalid number of acks to handler and a test
>
>
> merging with current trunk
>
>
> moved check for legal requiredAcks to append and fixed the tests accordingly
>
>
> 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 a6107b818947d6d6818c85cdffcb2b13f69a55c0
> clients/src/main/java/org/apache/kafka/common/protocol/Errors.java a8deac4ce5149129d0a6f44c0526af9d55649a36
> core/src/main/scala/kafka/cluster/Partition.scala e6ad8be5e33b6fb31c078ad78f8de709869ddc04
> core/src/main/scala/kafka/server/KafkaApis.scala 6ee7d8819a9ef923f3a65c865a0a3d8ded8845f0
> core/src/main/scala/kafka/server/ReplicaManager.scala fb948b9ab28c516e81dab14dcbe211dcd99842b6
> core/src/test/scala/unit/kafka/server/ReplicaManagerTest.scala faa907131ed0aa94a7eacb78c1ffb576062be87a
>
> Diff: https://reviews.apache.org/r/29647/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Gwen Shapira
>
>
Re: Review Request 29647: Patch for KAFKA-1697
Posted by Joel Koshy <jj...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29647/#review71960
-----------------------------------------------------------
clients/src/main/java/org/apache/kafka/common/errors/NotEnoughReplicasAfterAppendException.java
<https://reviews.apache.org/r/29647/#comment117874>
Pending discussion.
core/src/test/scala/unit/kafka/server/ReplicaManagerTest.scala
<https://reviews.apache.org/r/29647/#comment117872>
Can we just do the assert here instead of throwing an exception?
i.e., `assertEquals(responseStatus.values.size, responseStatus.values.count(_.error == INVALID_REQUIRED_ACKS))`
Either way is fine. Whichever is clearer although the above may be more concise if it works.
core/src/test/scala/unit/kafka/server/ReplicaManagerTest.scala
<https://reviews.apache.org/r/29647/#comment117873>
Do we need this here?
- Joel Koshy
On Feb. 11, 2015, 1:06 a.m., Gwen Shapira wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29647/
> -----------------------------------------------------------
>
> (Updated Feb. 11, 2015, 1:06 a.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1697
> https://issues.apache.org/jira/browse/KAFKA-1697
>
>
> Repository: kafka
>
>
> Description
> -------
>
> added early handling of invalid number of acks to handler and a test
>
>
> merging with current trunk
>
>
> moved check for legal requiredAcks to append and fixed the tests accordingly
>
>
> 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 a6107b818947d6d6818c85cdffcb2b13f69a55c0
> clients/src/main/java/org/apache/kafka/common/protocol/Errors.java a8deac4ce5149129d0a6f44c0526af9d55649a36
> core/src/main/scala/kafka/cluster/Partition.scala e6ad8be5e33b6fb31c078ad78f8de709869ddc04
> core/src/main/scala/kafka/server/KafkaApis.scala 6ee7d8819a9ef923f3a65c865a0a3d8ded8845f0
> core/src/main/scala/kafka/server/ReplicaManager.scala fb948b9ab28c516e81dab14dcbe211dcd99842b6
> core/src/test/scala/unit/kafka/server/ReplicaManagerTest.scala faa907131ed0aa94a7eacb78c1ffb576062be87a
>
> 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>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29647/
-----------------------------------------------------------
(Updated Feb. 11, 2015, 1:06 a.m.)
Review request for kafka.
Bugs: KAFKA-1697
https://issues.apache.org/jira/browse/KAFKA-1697
Repository: kafka
Description (updated)
-------
added early handling of invalid number of acks to handler and a test
merging with current trunk
moved check for legal requiredAcks to append and fixed the tests accordingly
Diffs (updated)
-----
clients/src/main/java/org/apache/kafka/common/errors/InvalidRequiredAcksException.java PRE-CREATION
clients/src/main/java/org/apache/kafka/common/errors/NotEnoughReplicasAfterAppendException.java a6107b818947d6d6818c85cdffcb2b13f69a55c0
clients/src/main/java/org/apache/kafka/common/protocol/Errors.java a8deac4ce5149129d0a6f44c0526af9d55649a36
core/src/main/scala/kafka/cluster/Partition.scala e6ad8be5e33b6fb31c078ad78f8de709869ddc04
core/src/main/scala/kafka/server/KafkaApis.scala 6ee7d8819a9ef923f3a65c865a0a3d8ded8845f0
core/src/main/scala/kafka/server/ReplicaManager.scala fb948b9ab28c516e81dab14dcbe211dcd99842b6
core/src/test/scala/unit/kafka/server/ReplicaManagerTest.scala faa907131ed0aa94a7eacb78c1ffb576062be87a
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>.
-----------------------------------------------------------
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 (updated)
-------
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 (updated)
-----
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