You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by GitBox <gi...@apache.org> on 2021/10/01 09:57:23 UTC
[GitHub] [kafka] predatorray opened a new pull request #10525: KAFKA-7572: Producer should not send requests with negative partition id
predatorray opened a new pull request #10525:
URL: https://github.com/apache/kafka/pull/10525
This PR is for [KAFKA-7572](https://issues.apache.org/jira/browse/KAFKA-7572), which fixes the issue that producers will throw confusing exceptions when a custom Partitioner returns a negative partition. Since the PR #5858 is not followed by anyone currently, I reopen this one to continue the work.
### Committer Checklist (excluded from commit message)
- [ ] Verify design and implementation
- [ ] Verify test coverage and CI build status
- [ ] Verify documentation (including upgrade notes)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] guozhangwang commented on pull request #10525: KAFKA-7572: Producer should not send requests with negative partition id
Posted by GitBox <gi...@apache.org>.
guozhangwang commented on pull request #10525:
URL: https://github.com/apache/kafka/pull/10525#issuecomment-1031749531
Merged to trunk, thanks @predatorray for the patch!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] predatorray edited a comment on pull request #10525: KAFKA-7572: Producer should not send requests with negative partition id
Posted by GitBox <gi...@apache.org>.
predatorray edited a comment on pull request #10525:
URL: https://github.com/apache/kafka/pull/10525#issuecomment-818424067
Hi, @showuon. Thanks for your review!
As per your question, I just follow the suggestion from @viktorsomogyi in the previous pr #5858 . I think maybe a subclass of KafkaException will be better since it indicates something more like an implementation error. To be honest, I cannot find any existing classes that suit this error, so i create a new one. If you think it will be better to throw `IllegalArgumentException`, i will change the code so we will not have introduce another new KafkaException to the code base.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] predatorray commented on pull request #10525: KAFKA-7572: Producer should not send requests with negative partition id
Posted by GitBox <gi...@apache.org>.
predatorray commented on pull request #10525:
URL: https://github.com/apache/kafka/pull/10525#issuecomment-877575923
Hi @showuon. It looks like nobody else shared thought about this exception for some time. I have changed to use the IllegalArgumentException instead of the InvalidPartitionException in the latest commit. Please help me review the PR again. Thanks!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] predatorray closed pull request #10525: KAFKA-7572: Producer should not send requests with negative partition id
Posted by GitBox <gi...@apache.org>.
predatorray closed pull request #10525:
URL: https://github.com/apache/kafka/pull/10525
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] guozhangwang commented on pull request #10525: KAFKA-7572: Producer should not send requests with negative partition id
Posted by GitBox <gi...@apache.org>.
guozhangwang commented on pull request #10525:
URL: https://github.com/apache/kafka/pull/10525#issuecomment-1029565310
@predatorray Thanks for the PR. Just following @showuon 's comment, I thought about the existing `InvalidPartitionsException`, but that has some issues to be used by the producer, since it is currently extending `ApiException` which is used by the admin / raft controller as returned by the brokers, and using it in the producer would also be considered a public change. So I think for now just throwing the non-checked `illegal-argument` as a fatal error to crash the producer directly is fine.
The PR needs some rebasing at the moment, otherwise it looks good to me.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] predatorray commented on pull request #10525: KAFKA-7572: Producer should not send requests with negative partition id
Posted by GitBox <gi...@apache.org>.
predatorray commented on pull request #10525:
URL: https://github.com/apache/kafka/pull/10525#issuecomment-1029767796
@guozhangwang Thanks for the reply and the review. The change was rebased.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] showuon commented on pull request #10525: KAFKA-7572: Producer should not send requests with negative partition id
Posted by GitBox <gi...@apache.org>.
showuon commented on pull request #10525:
URL: https://github.com/apache/kafka/pull/10525#issuecomment-818440268
OK, let's see what other reviewer's thought.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] guozhangwang merged pull request #10525: KAFKA-7572: Producer should not send requests with negative partition id
Posted by GitBox <gi...@apache.org>.
guozhangwang merged pull request #10525:
URL: https://github.com/apache/kafka/pull/10525
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] guozhangwang commented on pull request #10525: KAFKA-7572: Producer should not send requests with negative partition id
Posted by GitBox <gi...@apache.org>.
guozhangwang commented on pull request #10525:
URL: https://github.com/apache/kafka/pull/10525#issuecomment-1030974945
Re-trigger jenkins.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] guozhangwang commented on a change in pull request #10525: KAFKA-7572: Producer should not send requests with negative partition id
Posted by GitBox <gi...@apache.org>.
guozhangwang commented on a change in pull request #10525:
URL: https://github.com/apache/kafka/pull/10525#discussion_r800263685
##########
File path: clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java
##########
@@ -1305,10 +1305,16 @@ private ClusterResourceListeners configureClusterResourceListeners(Serializer<K>
*/
private int partition(ProducerRecord<K, V> record, byte[] serializedKey, byte[] serializedValue, Cluster cluster) {
Integer partition = record.partition();
- return partition != null ?
- partition :
- partitioner.partition(
- record.topic(), record.key(), serializedKey, record.value(), serializedValue, cluster);
+ if (partition != null) {
+ return partition;
+ }
+
+ int customPartition = partitioner.partition(
+ record.topic(), record.key(), serializedKey, record.value(), serializedValue, cluster);
+ if (customPartition < 0) {
+ throw new IllegalArgumentException(String.format("Invalid partition: %d. Partition number should always be non-negative.", customPartition));
Review comment:
Could you make the error message to be more specific? E.g. "Partitioner generated invalid partition: %d.." to differentiate that a partitioner is used.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] predatorray commented on a change in pull request #10525: KAFKA-7572: Producer should not send requests with negative partition id
Posted by GitBox <gi...@apache.org>.
predatorray commented on a change in pull request #10525:
URL: https://github.com/apache/kafka/pull/10525#discussion_r800756743
##########
File path: clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java
##########
@@ -1305,10 +1305,16 @@ private ClusterResourceListeners configureClusterResourceListeners(Serializer<K>
*/
private int partition(ProducerRecord<K, V> record, byte[] serializedKey, byte[] serializedValue, Cluster cluster) {
Integer partition = record.partition();
- return partition != null ?
- partition :
- partitioner.partition(
- record.topic(), record.key(), serializedKey, record.value(), serializedValue, cluster);
+ if (partition != null) {
+ return partition;
+ }
+
+ int customPartition = partitioner.partition(
+ record.topic(), record.key(), serializedKey, record.value(), serializedValue, cluster);
+ if (customPartition < 0) {
+ throw new IllegalArgumentException(String.format("Invalid partition: %d. Partition number should always be non-negative.", customPartition));
Review comment:
Sure. I have pushed a new commit. How about this one?
```java
"The partitioner generated an invalid partition number: %d. Partition number should always be non-negative."
```
By the way, I think Jenkins PR check did not run quite well. The failed tests are not related to this change. I tried some of these. Looks like they could be passed on my laptop.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] predatorray commented on pull request #10525: KAFKA-7572: Producer should not send requests with negative partition id
Posted by GitBox <gi...@apache.org>.
predatorray commented on pull request #10525:
URL: https://github.com/apache/kafka/pull/10525#issuecomment-818424067
Hi, @showuon. Thanks for your review!
As per your question, I just follow the suggestion from @viktorsomogyi in the previous pr #5858 . I think maybe a subclass of KafkaException will be better since it indicates something more like an implementation error. To be honest, I cannot find any existing classes that suit this error, so i create a new one. If you think it will be better to throw `IllegalArgumentException` is better, i will change the code so we will not have introduce another new KafkaException to the code base.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] predatorray commented on pull request #10525: KAFKA-7572: Producer should not send requests with negative partition id
Posted by GitBox <gi...@apache.org>.
predatorray commented on pull request #10525:
URL: https://github.com/apache/kafka/pull/10525#issuecomment-855196926
@jjkoshy Could you help me review this pr and share some thoughts about the exception with us?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] predatorray commented on pull request #10525: KAFKA-7572: Producer should not send requests with negative partition id
Posted by GitBox <gi...@apache.org>.
predatorray commented on pull request #10525:
URL: https://github.com/apache/kafka/pull/10525#issuecomment-1021977962
@guozhangwang Would you mind reviewing this pr for me?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org