You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@kafka.apache.org by Jonathan Natkins <na...@cloudera.com> on 2014/08/13 01:42:59 UTC
Review Request 24620: Patch for KAFKA-1580
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24620/
-----------------------------------------------------------
Review request for kafka.
Bugs: KAFKA-1580
https://issues.apache.org/jira/browse/KAFKA-1580
Repository: kafka
Description
-------
KAFKA-1580 Reject producer requests to internal topics
Diffs
-----
core/src/main/scala/kafka/server/KafkaApis.scala bb94673c8f3079a4053e0a7d58689e69775cdf8b
core/src/test/scala/unit/kafka/producer/ProducerTest.scala dd71d81041e1baa3e3ebdbc38f60f247ebd65948
Diff: https://reviews.apache.org/r/24620/diff/
Testing
-------
Thanks,
Jonathan Natkins
Re: Review Request 24620: Patch for KAFKA-1580
Posted by Neha Narkhede <ne...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24620/#review50656
-----------------------------------------------------------
core/src/main/scala/kafka/server/KafkaApis.scala
<https://reviews.apache.org/r/24620/#comment88510>
It is worth adding this error code to the new producer protocol (org.apache.kafka.common.protocol.Errors). If we do this, the producer will get a specific error code notifying it that the request was dropped due to an invalid topic. Currently, in this patch, the client would receive an unknown error code
core/src/test/scala/unit/kafka/producer/ProducerTest.scala
<https://reviews.apache.org/r/24620/#comment88509>
I suspect this test will be more productive if written using the new producer. That way, we could verify the received error code more specifically to be InvalidTopicException.
- Neha Narkhede
On Aug. 12, 2014, 11:42 p.m., Jonathan Natkins wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24620/
> -----------------------------------------------------------
>
> (Updated Aug. 12, 2014, 11:42 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1580
> https://issues.apache.org/jira/browse/KAFKA-1580
>
>
> Repository: kafka
>
>
> Description
> -------
>
> KAFKA-1580 Reject producer requests to internal topics
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/server/KafkaApis.scala bb94673c8f3079a4053e0a7d58689e69775cdf8b
> core/src/test/scala/unit/kafka/producer/ProducerTest.scala dd71d81041e1baa3e3ebdbc38f60f247ebd65948
>
> Diff: https://reviews.apache.org/r/24620/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Jonathan Natkins
>
>
Re: Review Request 24620: Patch for KAFKA-1580
Posted by Joel Koshy <jj...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24620/#review50786
-----------------------------------------------------------
Ship it!
Ship It!
- Joel Koshy
On Aug. 15, 2014, 10:05 p.m., Jonathan Natkins wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24620/
> -----------------------------------------------------------
>
> (Updated Aug. 15, 2014, 10:05 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1580
> https://issues.apache.org/jira/browse/KAFKA-1580
>
>
> Repository: kafka
>
>
> Description
> -------
>
> KAFKA-1580 Reject producer requests to internal topics
>
>
> Diffs
> -----
>
> clients/src/main/java/org/apache/kafka/common/errors/InvalidTopicException.java PRE-CREATION
> clients/src/main/java/org/apache/kafka/common/protocol/Errors.java 3374bd98be8e565608c4e764ed10afdae383fb6f
> core/src/main/scala/kafka/common/ErrorMapping.scala 5559d26ba2b96059f719754a351fa4598ca8a70b
> core/src/main/scala/kafka/server/KafkaApis.scala bb94673c8f3079a4053e0a7d58689e69775cdf8b
> core/src/test/scala/integration/kafka/api/ProducerFailureHandlingTest.scala 789e74c776d99f204e0ef87eba1f55a3551abdf2
>
> Diff: https://reviews.apache.org/r/24620/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Jonathan Natkins
>
>
Re: Review Request 24620: Patch for KAFKA-1580
Posted by Jonathan Natkins <na...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24620/
-----------------------------------------------------------
(Updated Aug. 15, 2014, 10:05 p.m.)
Review request for kafka.
Bugs: KAFKA-1580
https://issues.apache.org/jira/browse/KAFKA-1580
Repository: kafka
Description
-------
KAFKA-1580 Reject producer requests to internal topics
Diffs (updated)
-----
clients/src/main/java/org/apache/kafka/common/errors/InvalidTopicException.java PRE-CREATION
clients/src/main/java/org/apache/kafka/common/protocol/Errors.java 3374bd98be8e565608c4e764ed10afdae383fb6f
core/src/main/scala/kafka/common/ErrorMapping.scala 5559d26ba2b96059f719754a351fa4598ca8a70b
core/src/main/scala/kafka/server/KafkaApis.scala bb94673c8f3079a4053e0a7d58689e69775cdf8b
core/src/test/scala/integration/kafka/api/ProducerFailureHandlingTest.scala 789e74c776d99f204e0ef87eba1f55a3551abdf2
Diff: https://reviews.apache.org/r/24620/diff/
Testing
-------
Thanks,
Jonathan Natkins
Re: Review Request 24620: Patch for KAFKA-1580
Posted by Joel Koshy <jj...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24620/#review50784
-----------------------------------------------------------
core/src/main/scala/kafka/server/KafkaApis.scala
<https://reviews.apache.org/r/24620/#comment88649>
Since we will be adding more internal topics over time, this should probably be:
&& !(isOffsetTopic && topicAndPartition.topic == OffsetManager.OffsetsTopicName)
We can fix that on check-in. Overall, the offset commit handling is a bit hacky and KAFKA-1583 should significantly improve the overall structure.
- Joel Koshy
On Aug. 15, 2014, 1:21 a.m., Jonathan Natkins wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24620/
> -----------------------------------------------------------
>
> (Updated Aug. 15, 2014, 1:21 a.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1580
> https://issues.apache.org/jira/browse/KAFKA-1580
>
>
> Repository: kafka
>
>
> Description
> -------
>
> KAFKA-1580 Reject producer requests to internal topics
>
>
> Diffs
> -----
>
> clients/src/main/java/org/apache/kafka/common/errors/InvalidTopicException.java PRE-CREATION
> clients/src/main/java/org/apache/kafka/common/protocol/Errors.java 3374bd98be8e565608c4e764ed10afdae383fb6f
> core/src/main/scala/kafka/common/ErrorMapping.scala 5559d26ba2b96059f719754a351fa4598ca8a70b
> core/src/main/scala/kafka/server/KafkaApis.scala bb94673c8f3079a4053e0a7d58689e69775cdf8b
> core/src/test/scala/integration/kafka/api/ProducerFailureHandlingTest.scala 789e74c776d99f204e0ef87eba1f55a3551abdf2
>
> Diff: https://reviews.apache.org/r/24620/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Jonathan Natkins
>
>
Re: Review Request 24620: Patch for KAFKA-1580
Posted by Neha Narkhede <ne...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24620/#review50698
-----------------------------------------------------------
Ship it!
Ship It!
- Neha Narkhede
On Aug. 15, 2014, 1:21 a.m., Jonathan Natkins wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24620/
> -----------------------------------------------------------
>
> (Updated Aug. 15, 2014, 1:21 a.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1580
> https://issues.apache.org/jira/browse/KAFKA-1580
>
>
> Repository: kafka
>
>
> Description
> -------
>
> KAFKA-1580 Reject producer requests to internal topics
>
>
> Diffs
> -----
>
> clients/src/main/java/org/apache/kafka/common/errors/InvalidTopicException.java PRE-CREATION
> clients/src/main/java/org/apache/kafka/common/protocol/Errors.java 3374bd98be8e565608c4e764ed10afdae383fb6f
> core/src/main/scala/kafka/common/ErrorMapping.scala 5559d26ba2b96059f719754a351fa4598ca8a70b
> core/src/main/scala/kafka/server/KafkaApis.scala bb94673c8f3079a4053e0a7d58689e69775cdf8b
> core/src/test/scala/integration/kafka/api/ProducerFailureHandlingTest.scala 789e74c776d99f204e0ef87eba1f55a3551abdf2
>
> Diff: https://reviews.apache.org/r/24620/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Jonathan Natkins
>
>
Re: Review Request 24620: Patch for KAFKA-1580
Posted by Jonathan Natkins <na...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24620/
-----------------------------------------------------------
(Updated Aug. 15, 2014, 1:21 a.m.)
Review request for kafka.
Bugs: KAFKA-1580
https://issues.apache.org/jira/browse/KAFKA-1580
Repository: kafka
Description
-------
KAFKA-1580 Reject producer requests to internal topics
Diffs (updated)
-----
clients/src/main/java/org/apache/kafka/common/errors/InvalidTopicException.java PRE-CREATION
clients/src/main/java/org/apache/kafka/common/protocol/Errors.java 3374bd98be8e565608c4e764ed10afdae383fb6f
core/src/main/scala/kafka/common/ErrorMapping.scala 5559d26ba2b96059f719754a351fa4598ca8a70b
core/src/main/scala/kafka/server/KafkaApis.scala bb94673c8f3079a4053e0a7d58689e69775cdf8b
core/src/test/scala/integration/kafka/api/ProducerFailureHandlingTest.scala 789e74c776d99f204e0ef87eba1f55a3551abdf2
Diff: https://reviews.apache.org/r/24620/diff/
Testing
-------
Thanks,
Jonathan Natkins
Re: Review Request 24620: Patch for KAFKA-1580
Posted by Jonathan Natkins <na...@cloudera.com>.
> On Aug. 15, 2014, 12:12 a.m., Neha Narkhede wrote:
> > clients/src/main/java/org/apache/kafka/common/protocol/Errors.java, line 56
> > <https://reviews.apache.org/r/24620/diff/3/?file=661064#file661064line56>
> >
> > I meant adding the existing InvalidTopicException here. I'm not sure if there is value in differentiting amongst all the reasons the topic is invalid. We could just return the generic InvalidTopicException.
> >
> > Sorry if my suggestion wasn't clear enough.
Yeah, I understood the suggestion, I just have some mixed feelings about giving a relatively generic error message. I guess it's sufficient to have a more specific error message on the server, but it's a little unfortunate that there's not a way to pass a customized error message back to a Producer when something goes wrong. Makes for a more frustrating debugging session down the line, which is why I changed this to be more specific. Thoughts?
- Jonathan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24620/#review50669
-----------------------------------------------------------
On Aug. 14, 2014, 11:56 p.m., Jonathan Natkins wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24620/
> -----------------------------------------------------------
>
> (Updated Aug. 14, 2014, 11:56 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1580
> https://issues.apache.org/jira/browse/KAFKA-1580
>
>
> Repository: kafka
>
>
> Description
> -------
>
> KAFKA-1580 Reject producer requests to internal topics
>
>
> Diffs
> -----
>
> clients/src/main/java/org/apache/kafka/common/errors/InternalTopicOperationException.java PRE-CREATION
> clients/src/main/java/org/apache/kafka/common/protocol/Errors.java 3374bd98be8e565608c4e764ed10afdae383fb6f
> core/src/main/scala/kafka/common/ErrorMapping.scala 5559d26ba2b96059f719754a351fa4598ca8a70b
> core/src/main/scala/kafka/common/InternalTopicOperationException.scala PRE-CREATION
> core/src/main/scala/kafka/server/KafkaApis.scala bb94673c8f3079a4053e0a7d58689e69775cdf8b
> core/src/test/scala/integration/kafka/api/ProducerFailureHandlingTest.scala 789e74c776d99f204e0ef87eba1f55a3551abdf2
>
> Diff: https://reviews.apache.org/r/24620/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Jonathan Natkins
>
>
Re: Review Request 24620: Patch for KAFKA-1580
Posted by Neha Narkhede <ne...@gmail.com>.
> On Aug. 15, 2014, 12:12 a.m., Neha Narkhede wrote:
> > clients/src/main/java/org/apache/kafka/common/protocol/Errors.java, line 56
> > <https://reviews.apache.org/r/24620/diff/3/?file=661064#file661064line56>
> >
> > I meant adding the existing InvalidTopicException here. I'm not sure if there is value in differentiting amongst all the reasons the topic is invalid. We could just return the generic InvalidTopicException.
> >
> > Sorry if my suggestion wasn't clear enough.
>
> Jonathan Natkins wrote:
> Yeah, I understood the suggestion, I just have some mixed feelings about giving a relatively generic error message. I guess it's sufficient to have a more specific error message on the server, but it's a little unfortunate that there's not a way to pass a customized error message back to a Producer when something goes wrong. Makes for a more frustrating debugging session down the line, which is why I changed this to be more specific. Thoughts?
>
> Jonathan Natkins wrote:
> That said, I believe revision 4 is the patch you were hoping for
Ya, that makes sense. Here are my general thoughts on this -
I guess there is a certain level of detail that would be good to pass onto the client. My guess is that knowing the error would matter more than the various ways in which you can hit the error. So for example, the topic name is invalid - now whether it is due to an illegal character or just a typo, shouldn't be the level of detail the client would worry about.
- Neha
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24620/#review50669
-----------------------------------------------------------
On Aug. 15, 2014, 1:21 a.m., Jonathan Natkins wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24620/
> -----------------------------------------------------------
>
> (Updated Aug. 15, 2014, 1:21 a.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1580
> https://issues.apache.org/jira/browse/KAFKA-1580
>
>
> Repository: kafka
>
>
> Description
> -------
>
> KAFKA-1580 Reject producer requests to internal topics
>
>
> Diffs
> -----
>
> clients/src/main/java/org/apache/kafka/common/errors/InvalidTopicException.java PRE-CREATION
> clients/src/main/java/org/apache/kafka/common/protocol/Errors.java 3374bd98be8e565608c4e764ed10afdae383fb6f
> core/src/main/scala/kafka/common/ErrorMapping.scala 5559d26ba2b96059f719754a351fa4598ca8a70b
> core/src/main/scala/kafka/server/KafkaApis.scala bb94673c8f3079a4053e0a7d58689e69775cdf8b
> core/src/test/scala/integration/kafka/api/ProducerFailureHandlingTest.scala 789e74c776d99f204e0ef87eba1f55a3551abdf2
>
> Diff: https://reviews.apache.org/r/24620/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Jonathan Natkins
>
>
Re: Review Request 24620: Patch for KAFKA-1580
Posted by Jonathan Natkins <na...@cloudera.com>.
> On Aug. 15, 2014, 12:12 a.m., Neha Narkhede wrote:
> > clients/src/main/java/org/apache/kafka/common/protocol/Errors.java, line 56
> > <https://reviews.apache.org/r/24620/diff/3/?file=661064#file661064line56>
> >
> > I meant adding the existing InvalidTopicException here. I'm not sure if there is value in differentiting amongst all the reasons the topic is invalid. We could just return the generic InvalidTopicException.
> >
> > Sorry if my suggestion wasn't clear enough.
>
> Jonathan Natkins wrote:
> Yeah, I understood the suggestion, I just have some mixed feelings about giving a relatively generic error message. I guess it's sufficient to have a more specific error message on the server, but it's a little unfortunate that there's not a way to pass a customized error message back to a Producer when something goes wrong. Makes for a more frustrating debugging session down the line, which is why I changed this to be more specific. Thoughts?
That said, I believe revision 4 is the patch you were hoping for
- Jonathan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24620/#review50669
-----------------------------------------------------------
On Aug. 15, 2014, 1:21 a.m., Jonathan Natkins wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24620/
> -----------------------------------------------------------
>
> (Updated Aug. 15, 2014, 1:21 a.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1580
> https://issues.apache.org/jira/browse/KAFKA-1580
>
>
> Repository: kafka
>
>
> Description
> -------
>
> KAFKA-1580 Reject producer requests to internal topics
>
>
> Diffs
> -----
>
> clients/src/main/java/org/apache/kafka/common/errors/InvalidTopicException.java PRE-CREATION
> clients/src/main/java/org/apache/kafka/common/protocol/Errors.java 3374bd98be8e565608c4e764ed10afdae383fb6f
> core/src/main/scala/kafka/common/ErrorMapping.scala 5559d26ba2b96059f719754a351fa4598ca8a70b
> core/src/main/scala/kafka/server/KafkaApis.scala bb94673c8f3079a4053e0a7d58689e69775cdf8b
> core/src/test/scala/integration/kafka/api/ProducerFailureHandlingTest.scala 789e74c776d99f204e0ef87eba1f55a3551abdf2
>
> Diff: https://reviews.apache.org/r/24620/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Jonathan Natkins
>
>
Re: Review Request 24620: Patch for KAFKA-1580
Posted by Neha Narkhede <ne...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24620/#review50669
-----------------------------------------------------------
clients/src/main/java/org/apache/kafka/common/protocol/Errors.java
<https://reviews.apache.org/r/24620/#comment88525>
I meant adding the existing InvalidTopicException here. I'm not sure if there is value in differentiting amongst all the reasons the topic is invalid. We could just return the generic InvalidTopicException.
Sorry if my suggestion wasn't clear enough.
- Neha Narkhede
On Aug. 14, 2014, 11:56 p.m., Jonathan Natkins wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24620/
> -----------------------------------------------------------
>
> (Updated Aug. 14, 2014, 11:56 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1580
> https://issues.apache.org/jira/browse/KAFKA-1580
>
>
> Repository: kafka
>
>
> Description
> -------
>
> KAFKA-1580 Reject producer requests to internal topics
>
>
> Diffs
> -----
>
> clients/src/main/java/org/apache/kafka/common/errors/InternalTopicOperationException.java PRE-CREATION
> clients/src/main/java/org/apache/kafka/common/protocol/Errors.java 3374bd98be8e565608c4e764ed10afdae383fb6f
> core/src/main/scala/kafka/common/ErrorMapping.scala 5559d26ba2b96059f719754a351fa4598ca8a70b
> core/src/main/scala/kafka/common/InternalTopicOperationException.scala PRE-CREATION
> core/src/main/scala/kafka/server/KafkaApis.scala bb94673c8f3079a4053e0a7d58689e69775cdf8b
> core/src/test/scala/integration/kafka/api/ProducerFailureHandlingTest.scala 789e74c776d99f204e0ef87eba1f55a3551abdf2
>
> Diff: https://reviews.apache.org/r/24620/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Jonathan Natkins
>
>
Re: Review Request 24620: Patch for KAFKA-1580
Posted by Jonathan Natkins <na...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24620/
-----------------------------------------------------------
(Updated Aug. 14, 2014, 11:56 p.m.)
Review request for kafka.
Bugs: KAFKA-1580
https://issues.apache.org/jira/browse/KAFKA-1580
Repository: kafka
Description
-------
KAFKA-1580 Reject producer requests to internal topics
Diffs (updated)
-----
clients/src/main/java/org/apache/kafka/common/errors/InternalTopicOperationException.java PRE-CREATION
clients/src/main/java/org/apache/kafka/common/protocol/Errors.java 3374bd98be8e565608c4e764ed10afdae383fb6f
core/src/main/scala/kafka/common/ErrorMapping.scala 5559d26ba2b96059f719754a351fa4598ca8a70b
core/src/main/scala/kafka/common/InternalTopicOperationException.scala PRE-CREATION
core/src/main/scala/kafka/server/KafkaApis.scala bb94673c8f3079a4053e0a7d58689e69775cdf8b
core/src/test/scala/integration/kafka/api/ProducerFailureHandlingTest.scala 789e74c776d99f204e0ef87eba1f55a3551abdf2
Diff: https://reviews.apache.org/r/24620/diff/
Testing
-------
Thanks,
Jonathan Natkins
Re: Review Request 24620: Patch for KAFKA-1580
Posted by Jonathan Natkins <na...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24620/
-----------------------------------------------------------
(Updated Aug. 14, 2014, 11:50 p.m.)
Review request for kafka.
Bugs: KAFKA-1580
https://issues.apache.org/jira/browse/KAFKA-1580
Repository: kafka
Description
-------
KAFKA-1580 Reject producer requests to internal topics
Diffs (updated)
-----
clients/src/main/java/org/apache/kafka/common/errors/InternalTopicOperationException.java PRE-CREATION
clients/src/main/java/org/apache/kafka/common/protocol/Errors.java 3374bd98be8e565608c4e764ed10afdae383fb6f
core/src/main/scala/kafka/common/ErrorMapping.scala 5559d26ba2b96059f719754a351fa4598ca8a70b
core/src/main/scala/kafka/common/InternalTopicOperationException.scala PRE-CREATION
core/src/main/scala/kafka/server/KafkaApis.scala bb94673c8f3079a4053e0a7d58689e69775cdf8b
core/src/test/scala/integration/kafka/api/ProducerFailureHandlingTest.scala 789e74c776d99f204e0ef87eba1f55a3551abdf2
core/src/test/scala/unit/kafka/producer/ProducerTest.scala dd71d81041e1baa3e3ebdbc38f60f247ebd65948
Diff: https://reviews.apache.org/r/24620/diff/
Testing
-------
Thanks,
Jonathan Natkins