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/05/29 23:18:37 UTC
[GitHub] [kafka] mjsax opened a new pull request #8759: KAFKA-10066: TestOutputTopic should pass record headers into deserializers
mjsax opened a new pull request #8759:
URL: https://github.com/apache/kafka/pull/8759
Call for review @vvcephei
----------------------------------------------------------------
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 #8759: KAFKA-10066: TestOutputTopic should pass record headers into deserializers
Posted by GitBox <gi...@apache.org>.
mjsax commented on pull request #8759:
URL: https://github.com/apache/kafka/pull/8759#issuecomment-639170115
Merged to `trunk` and cherry-picked to `2.6` and `2.5` branches.
----------------------------------------------------------------
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 #8759: KAFKA-10066: TestOutputTopic should pass record headers into deserializers
Posted by GitBox <gi...@apache.org>.
mjsax commented on a change in pull request #8759:
URL: https://github.com/apache/kafka/pull/8759#discussion_r433564798
##########
File path: streams/test-utils/src/main/java/org/apache/kafka/streams/TopologyTestDriver.java
##########
@@ -906,8 +906,8 @@ public void advanceWallClockTime(final Duration advance) {
if (record == null) {
throw new NoSuchElementException("Empty topic: " + topic);
}
- final K key = keyDeserializer.deserialize(record.topic(), record.key());
- final V value = valueDeserializer.deserialize(record.topic(), record.value());
+ final K key = keyDeserializer.deserialize(record.topic(), record.headers(), record.key());
Review comment:
Not sure if I can follow? `readOutput` is marked as deprecated: https://github.com/apache/kafka/blob/trunk/streams/test-utils/src/main/java/org/apache/kafka/streams/TopologyTestDriver.java#L797
I can still fix it on the side, but nobody should use it any longer and thus the gain seems minimal.
----------------------------------------------------------------
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 #8759: KAFKA-10066: TestOutputTopic should pass record headers into deserializers
Posted by GitBox <gi...@apache.org>.
mjsax commented on pull request #8759:
URL: https://github.com/apache/kafka/pull/8759#issuecomment-638512007
Java 11:
```
kafka.api.TransactionsBounceTest.testWithGroupMetadata
org.apache.kafka.clients.consumer.CooperativeStickyAssignorTest.testReassignmentWithRandomSubscriptionsAndChanges
org.apache.kafka.clients.consumer.CooperativeStickyAssignorTest.testReassignmentWithRandomSubscriptionsAndChanges
org.apache.kafka.clients.consumer.StickyAssignorTest.testReassignmentWithRandomSubscriptionsAndChanges
org.apache.kafka.clients.consumer.StickyAssignorTest.testReassignmentWithRandomSubscriptionsAndChanges
```
Java 14:
```
org.apache.kafka.clients.consumer.CooperativeStickyAssignorTest.testReassignmentWithRandomSubscriptionsAndChanges
org.apache.kafka.clients.consumer.StickyAssignorTest.testReassignmentWithRandomSubscriptionsAndChanges
```
Java 8:
```
org.apache.kafka.clients.consumer.CooperativeStickyAssignorTest.testReassignmentWithRandomSubscriptionsAndChanges
org.apache.kafka.clients.consumer.CooperativeStickyAssignorTest.testReassignmentWithRandomSubscriptionsAndChanges
org.apache.kafka.clients.consumer.StickyAssignorTest.testReassignmentWithRandomSubscriptionsAndChanges
org.apache.kafka.clients.consumer.StickyAssignorTest.testReassignmentWithRandomSubscriptionsAndChanges
```
----------------------------------------------------------------
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] chia7712 commented on a change in pull request #8759: KAFKA-10066: TestOutputTopic should pass record headers into deserializers
Posted by GitBox <gi...@apache.org>.
chia7712 commented on a change in pull request #8759:
URL: https://github.com/apache/kafka/pull/8759#discussion_r433596501
##########
File path: streams/test-utils/src/main/java/org/apache/kafka/streams/TopologyTestDriver.java
##########
@@ -906,8 +906,8 @@ public void advanceWallClockTime(final Duration advance) {
if (record == null) {
throw new NoSuchElementException("Empty topic: " + topic);
}
- final K key = keyDeserializer.deserialize(record.topic(), record.key());
- final V value = valueDeserializer.deserialize(record.topic(), record.value());
+ final K key = keyDeserializer.deserialize(record.topic(), record.headers(), record.key());
Review comment:
> but nobody should use it any longer and thus the gain seems minimal.
You are right. The benefit is too low.
----------------------------------------------------------------
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 #8759: KAFKA-10066: TestOutputTopic should pass record headers into deserializers
Posted by GitBox <gi...@apache.org>.
mjsax commented on a change in pull request #8759:
URL: https://github.com/apache/kafka/pull/8759#discussion_r433055097
##########
File path: streams/test-utils/src/test/java/org/apache/kafka/streams/TopologyTestDriverTest.java
##########
@@ -711,6 +715,56 @@ public void shouldUseSourceSpecificDeserializers() {
assertThat(result2.getValue(), equalTo(source2Value));
}
+ @Test
+ public void shouldPassRecordHeadersIntoDeSerializers() {
Review comment:
The capital `S` was on purpose; it's short for `IntoDeserializersAndSerializers`
----------------------------------------------------------------
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 #8759: KAFKA-10066: TestOutputTopic should pass record headers into deserializers
Posted by GitBox <gi...@apache.org>.
mjsax commented on a change in pull request #8759:
URL: https://github.com/apache/kafka/pull/8759#discussion_r433055994
##########
File path: streams/test-utils/src/test/java/org/apache/kafka/streams/TopologyTestDriverTest.java
##########
@@ -711,6 +715,56 @@ public void shouldUseSourceSpecificDeserializers() {
assertThat(result2.getValue(), equalTo(source2Value));
}
+ @Test
+ public void shouldPassRecordHeadersIntoDeSerializers() {
Review comment:
We don't verify that header are deserialized -- we just verify that the right function is called, that hands the header into the deserializer (ie, to allow used to read them as metadata).
I guess we should use mocks for the deserializers, but not for the serializers. Thus, not using mocks make the test use the same patter for both what seems "nicer".
> Moreover, we could build a customized boolean deserializer which outputs true when the header is provided, or false otherwise.
Would this make the test simpler? I personally doubt it.
----------------------------------------------------------------
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 #8759: KAFKA-10066: TestOutputTopic should pass record headers into deserializers
Posted by GitBox <gi...@apache.org>.
mjsax commented on pull request #8759:
URL: https://github.com/apache/kafka/pull/8759#issuecomment-637856511
----------------------------------------------------------------
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 #8759: KAFKA-10066: TestOutputTopic should pass record headers into deserializers
Posted by GitBox <gi...@apache.org>.
mjsax commented on pull request #8759:
URL: https://github.com/apache/kafka/pull/8759#issuecomment-637796962
----------------------------------------------------------------
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 #8759: KAFKA-10066: TestOutputTopic should pass record headers into deserializers
Posted by GitBox <gi...@apache.org>.
mjsax commented on a change in pull request #8759:
URL: https://github.com/apache/kafka/pull/8759#discussion_r433054885
##########
File path: streams/test-utils/src/main/java/org/apache/kafka/streams/TopologyTestDriver.java
##########
@@ -906,8 +906,8 @@ public void advanceWallClockTime(final Duration advance) {
if (record == null) {
throw new NoSuchElementException("Empty topic: " + topic);
}
- final K key = keyDeserializer.deserialize(record.topic(), record.key());
- final V value = valueDeserializer.deserialize(record.topic(), record.value());
+ final K key = keyDeserializer.deserialize(record.topic(), record.headers(), record.key());
Review comment:
`readOutput` is deprecated. Thus not sure if it's worth to fix?
----------------------------------------------------------------
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] abbccdda commented on a change in pull request #8759: KAFKA-10066: TestOutputTopic should pass record headers into deserializers
Posted by GitBox <gi...@apache.org>.
abbccdda commented on a change in pull request #8759:
URL: https://github.com/apache/kafka/pull/8759#discussion_r432901618
##########
File path: streams/test-utils/src/test/java/org/apache/kafka/streams/TopologyTestDriverTest.java
##########
@@ -711,6 +715,56 @@ public void shouldUseSourceSpecificDeserializers() {
assertThat(result2.getValue(), equalTo(source2Value));
}
+ @Test
+ public void shouldPassRecordHeadersIntoDeSerializers() {
Review comment:
It seems like we are just verifying the headers are deserialized. Should we just use mock deserializers? Moreover, we could build a customized `boolean` deserializer which outputs true when the header is provided, or false otherwise.
##########
File path: streams/test-utils/src/test/java/org/apache/kafka/streams/TopologyTestDriverTest.java
##########
@@ -711,6 +715,56 @@ public void shouldUseSourceSpecificDeserializers() {
assertThat(result2.getValue(), equalTo(source2Value));
}
+ @Test
+ public void shouldPassRecordHeadersIntoDeSerializers() {
Review comment:
DeSerializers -> Deserializers
----------------------------------------------------------------
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] chia7712 commented on a change in pull request #8759: KAFKA-10066: TestOutputTopic should pass record headers into deserializers
Posted by GitBox <gi...@apache.org>.
chia7712 commented on a change in pull request #8759:
URL: https://github.com/apache/kafka/pull/8759#discussion_r433364776
##########
File path: streams/test-utils/src/main/java/org/apache/kafka/streams/TopologyTestDriver.java
##########
@@ -906,8 +906,8 @@ public void advanceWallClockTime(final Duration advance) {
if (record == null) {
throw new NoSuchElementException("Empty topic: " + topic);
}
- final K key = keyDeserializer.deserialize(record.topic(), record.key());
- final V value = valueDeserializer.deserialize(record.topic(), record.value());
+ final K key = keyDeserializer.deserialize(record.topic(), record.headers(), record.key());
Review comment:
> Thus not sure if it's worth to fix?
Personally, what we should add to ```readOutput``` is "deprecation" rather than "a bug". Hence, it is worthwhile to fix if the fix does not break anything.
----------------------------------------------------------------
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 #8759: KAFKA-10066: TestOutputTopic should pass record headers into deserializers
Posted by GitBox <gi...@apache.org>.
mjsax merged pull request #8759:
URL: https://github.com/apache/kafka/pull/8759
----------------------------------------------------------------
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] chia7712 commented on a change in pull request #8759: KAFKA-10066: TestOutputTopic should pass record headers into deserializers
Posted by GitBox <gi...@apache.org>.
chia7712 commented on a change in pull request #8759:
URL: https://github.com/apache/kafka/pull/8759#discussion_r432919211
##########
File path: streams/test-utils/src/main/java/org/apache/kafka/streams/TopologyTestDriver.java
##########
@@ -906,8 +906,8 @@ public void advanceWallClockTime(final Duration advance) {
if (record == null) {
throw new NoSuchElementException("Empty topic: " + topic);
}
- final K key = keyDeserializer.deserialize(record.topic(), record.key());
- final V value = valueDeserializer.deserialize(record.topic(), record.value());
+ final K key = keyDeserializer.deserialize(record.topic(), record.headers(), record.key());
Review comment:
Nice finding!
https://github.com/apache/kafka/blob/trunk/streams/test-utils/src/main/java/org/apache/kafka/streams/TopologyTestDriver.java#L805 has similar issue. Should we fix it as well?
----------------------------------------------------------------
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