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 2020/09/19 14:04:18 UTC
[GitHub] [kafka] gmunozfe opened a new pull request #9309: [KAFKA-10503] MockProducer doesn't throw ClassCastException when no
gmunozfe opened a new pull request #9309:
URL: https://github.com/apache/kafka/pull/9309
partition for topic
*More detailed description of your change,
if necessary. The PR title and PR message become
the squashed commit message, so use a separate
comment to ping reviewers.*
Though MockProducer admits serializers in its constructors, it doesn't check during send method that those serializers are the proper ones to serialize key/value included into the ProducerRecord.
This check is only done if there is a partition assigned for that topic.
It would be an enhancement if these serialize methods were also invoked in simple scenarios, where no partition is assigned to a topic.
*Summary of testing strategy (including rationale)
for the feature or bug fix. Unit and/or integration
tests are expected for any behaviour change and
system tests should be considered for larger changes.*
Added a unit test to check expected behaviour
### 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] mjsax commented on pull request #9309: KAFKA-10503: MockProducer doesn't throw ClassCastException when no
Posted by GitBox <gi...@apache.org>.
mjsax commented on pull request #9309:
URL: https://github.com/apache/kafka/pull/9309#issuecomment-701896013
Thanks for the PR @gmunozfe. Merged to `trunk`.
----------------------------------------------------------------
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] mjsax commented on a change in pull request #9309: KAFKA-10503: MockProducer doesn't throw ClassCastException when no
Posted by GitBox <gi...@apache.org>.
mjsax commented on a change in pull request #9309:
URL: https://github.com/apache/kafka/pull/9309#discussion_r497991052
##########
File path: clients/src/test/java/org/apache/kafka/clients/producer/MockProducerTest.java
##########
@@ -765,6 +766,16 @@ public void shouldThrowOnFlushProducerIfProducerIsClosed() {
fail("Should have thrown as producer is already closed");
} catch (IllegalStateException e) { }
}
+
+ @Test
+ @SuppressWarnings("unchecked")
+ public void shouldThrowClassCastException() {
+ try (MockProducer<Integer, String> producer = new MockProducer<>(true, new IntegerSerializer(), new StringSerializer());) {
+ ProducerRecord record = new ProducerRecord(topic, "key1", "value1");
+ producer.send(record);
+ fail("Should have thrown ClassCastException because record cannot be casted with serializers");
Review comment:
Oh, I see. We actually want to pass in an incorrect key-type to get the `ClassCastException`. So you are right, for this case we cannot specify the generics. Keeping the code as-is an to suppress the warning is fine than. Totally missed this.
----------------------------------------------------------------
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] mjsax commented on a change in pull request #9309: KAFKA-10503: MockProducer doesn't throw ClassCastException when no
Posted by GitBox <gi...@apache.org>.
mjsax commented on a change in pull request #9309:
URL: https://github.com/apache/kafka/pull/9309#discussion_r497991542
##########
File path: clients/src/test/java/org/apache/kafka/clients/producer/MockProducerTest.java
##########
@@ -765,6 +766,16 @@ public void shouldThrowOnFlushProducerIfProducerIsClosed() {
fail("Should have thrown as producer is already closed");
} catch (IllegalStateException e) { }
}
+
+ @Test
+ @SuppressWarnings("unchecked")
+ public void shouldThrowClassCastException() {
+ try (MockProducer<Integer, String> producer = new MockProducer<>(true, new IntegerSerializer(), new StringSerializer());) {
+ ProducerRecord record = new ProducerRecord(topic, "key1", "value1");
+ producer.send(record);
+ fail("Should have thrown ClassCastException because record cannot be casted with serializers");
Review comment:
Yes, we used an older version of the test libraries and thus existing code uses a different pattern. We just try to use the newer (and better) pattern in new code and migrate existing code lazily to the new pattern.
----------------------------------------------------------------
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] mjsax commented on a change in pull request #9309: KAFKA-10503: MockProducer doesn't throw ClassCastException when no
Posted by GitBox <gi...@apache.org>.
mjsax commented on a change in pull request #9309:
URL: https://github.com/apache/kafka/pull/9309#discussion_r497712541
##########
File path: clients/src/test/java/org/apache/kafka/clients/producer/MockProducerTest.java
##########
@@ -765,6 +766,16 @@ public void shouldThrowOnFlushProducerIfProducerIsClosed() {
fail("Should have thrown as producer is already closed");
} catch (IllegalStateException e) { }
}
+
+ @Test
+ @SuppressWarnings("unchecked")
Review comment:
We should avoid to suppress warning. Seems, you need to add generics to `ProducerRecord` in L774 to fix it?
##########
File path: clients/src/test/java/org/apache/kafka/clients/producer/MockProducerTest.java
##########
@@ -765,6 +766,16 @@ public void shouldThrowOnFlushProducerIfProducerIsClosed() {
fail("Should have thrown as producer is already closed");
} catch (IllegalStateException e) { }
}
+
+ @Test
+ @SuppressWarnings("unchecked")
+ public void shouldThrowClassCastException() {
+ try (MockProducer<Integer, String> producer = new MockProducer<>(true, new IntegerSerializer(), new StringSerializer());) {
+ ProducerRecord record = new ProducerRecord(topic, "key1", "value1");
+ producer.send(record);
+ fail("Should have thrown ClassCastException because record cannot be casted with serializers");
Review comment:
instead of `fail` we should use ```
assertThrows(
ClassCastException.class,
() -> producer.send(record);
);
```
For this case, we can remove the `catch` block below.
----------------------------------------------------------------
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] gmunozfe commented on a change in pull request #9309: KAFKA-10503: MockProducer doesn't throw ClassCastException when no
Posted by GitBox <gi...@apache.org>.
gmunozfe commented on a change in pull request #9309:
URL: https://github.com/apache/kafka/pull/9309#discussion_r497840533
##########
File path: clients/src/test/java/org/apache/kafka/clients/producer/MockProducerTest.java
##########
@@ -765,6 +766,16 @@ public void shouldThrowOnFlushProducerIfProducerIsClosed() {
fail("Should have thrown as producer is already closed");
} catch (IllegalStateException e) { }
}
+
+ @Test
+ @SuppressWarnings("unchecked")
+ public void shouldThrowClassCastException() {
+ try (MockProducer<Integer, String> producer = new MockProducer<>(true, new IntegerSerializer(), new StringSerializer());) {
+ ProducerRecord record = new ProducerRecord(topic, "key1", "value1");
+ producer.send(record);
+ fail("Should have thrown ClassCastException because record cannot be casted with serializers");
Review comment:
Done, I have followed the same approach that other tests in the same class, but assertThrows is also a good alternative
----------------------------------------------------------------
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] mjsax commented on a change in pull request #9309: KAFKA-10503: MockProducer doesn't throw ClassCastException when no
Posted by GitBox <gi...@apache.org>.
mjsax commented on a change in pull request #9309:
URL: https://github.com/apache/kafka/pull/9309#discussion_r497991224
##########
File path: clients/src/test/java/org/apache/kafka/clients/producer/MockProducerTest.java
##########
@@ -765,6 +766,16 @@ public void shouldThrowOnFlushProducerIfProducerIsClosed() {
fail("Should have thrown as producer is already closed");
} catch (IllegalStateException e) { }
}
+
+ @Test
+ @SuppressWarnings("unchecked")
Review comment:
Oh, I see. We actually want to pass in an incorrect key-type to get the ClassCastException. So you are right, for this case we cannot specify the generics. Keeping the code as-is and to suppress the warning is fine than. Totally missed this. Thanks for clarifying.
----------------------------------------------------------------
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] mjsax merged pull request #9309: KAFKA-10503: MockProducer doesn't throw ClassCastException when no
Posted by GitBox <gi...@apache.org>.
mjsax merged pull request #9309:
URL: https://github.com/apache/kafka/pull/9309
----------------------------------------------------------------
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] gmunozfe commented on a change in pull request #9309: KAFKA-10503: MockProducer doesn't throw ClassCastException when no
Posted by GitBox <gi...@apache.org>.
gmunozfe commented on a change in pull request #9309:
URL: https://github.com/apache/kafka/pull/9309#discussion_r497840118
##########
File path: clients/src/test/java/org/apache/kafka/clients/producer/MockProducerTest.java
##########
@@ -765,6 +766,16 @@ public void shouldThrowOnFlushProducerIfProducerIsClosed() {
fail("Should have thrown as producer is already closed");
} catch (IllegalStateException e) { }
}
+
+ @Test
+ @SuppressWarnings("unchecked")
Review comment:
Thanks a lot for your review @mjsax
If I add generics, bounded type parameters will make compilation fail, as types are checked and incompatible.
In fact, the purpose of this enhancement is that MockProducer throws the exception when types are not restricted, wdyt?
----------------------------------------------------------------
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] gmunozfe commented on pull request #9309: KAFKA-10503: MockProducer doesn't throw ClassCastException when no
Posted by GitBox <gi...@apache.org>.
gmunozfe commented on pull request #9309:
URL: https://github.com/apache/kafka/pull/9309#issuecomment-696995620
@mjsax @guozhangwang @leonardge could you review? 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] gmunozfe commented on pull request #9309: KAFKA-10503: MockProducer doesn't throw ClassCastException when no
Posted by GitBox <gi...@apache.org>.
gmunozfe commented on pull request #9309:
URL: https://github.com/apache/kafka/pull/9309#issuecomment-696995620
@mjsax @guozhangwang @leonardge could you review? 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org