KAFKA-10066: TestOutputTopic should pass record headers into deserializers by mjsax · Pull Request #8759 · apache/kafka

mjsax · 2020-05-29T23:18:26Z

Call for review @vvcephei

vvcephei

Thanks for this quick fix, @mjsax

abbccdda · 2020-05-31T01:42:22Z

DeSerializers -> Deserializers

The capital S was on purpose; it's short for IntoDeserializersAndSerializers

abbccdda · 2020-05-31T01:46:21Z

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.

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.

chia7712 · 2020-05-31T07:34:26Z

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?

readOutput is deprecated. Thus not sure if it's worth to fix?

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.

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.

but nobody should use it any longer and thus the gain seems minimal.

You are right. The benefit is too low.

mjsax · 2020-06-02T20:47:16Z

Build failed due to broken trunk; already fixed.

mjsax · 2020-06-02T20:47:25Z

Retest this please.

mjsax · 2020-06-02T23:17:49Z

Java 11:

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

Those test should be fixed already.

mjsax · 2020-06-02T23:17:56Z

Retest this please

mjsax · 2020-06-03T23:24:48Z

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

deserializers

…izers (#8759) Reviewers: John Roesler <john@confluent.io>, Boyang Chen <boyang@confluent.io>, Chia-Ping Tsai <chia7712@gmail.com>

mjsax · 2020-06-04T23:30:29Z

Merged to trunk and cherry-picked to 2.6 and 2.5 branches.

* 'trunk' of github.com:apache/kafka: (46 commits) MINOR: improve code encapsulation between StreamThread and TaskManager (apache#8819) Fixing KAFKA-10094 (apache#8797) KAFKA-9851: Revoking Connect tasks due to connectivity issues should also clear the running assignment (apache#8804) KAFKA-9840; Skip End Offset validation when the leader epoch is not reliable (apache#8486) HOT_FIX: Update javadoc since imports added (apache#8817) KAFKA-8011: Fix flaky RegexSourceIntegrationTest (apache#8799) KAFKA-9570: Define SSL configs in all worker config classes, not just distributed (apache#8135) KAFKA-10111: Make SinkTaskContext.errantRecordReporter() a default method (apache#8814) KAFKA-10110: Corrected potential NPE when null label value added to KafkaMetricsContext (apache#8811) MINOR: Change the order that Connect calls `config()` and `validate()` to avoid validating if the required ConfigDef is null (apache#8810) MINOR: fix backwards incompatibility in JmxReporter introduced by KIP-606 MINOR: Fix javadoc warnings (apache#8809) KAFKA-9441: Improve Kafka Streams task management (apache#8776) fix the broken links of streams javadoc (apache#8789) KAFKA-10040; Make computing the PreferredReplicaImbalanceCount metric more efficient (apache#8724) KAFKA-10066: TestOutputTopic should pass record headers into deserializers (apache#8759) MINOR: Add explanation for disabling forwarding from value transformers (apache#8771) KAFKA-10033: Throw UnknownTopicOrPartitionException if altering configs of non-existing topic KAFKA-9434: automated protocol for alterReplicaLogDirs (apache#8311) KAFKA-9313: Set `use_all_dns_ips` as the new default for `client.dns.lookup` (KIP-602) (apache#8644) ...

mjsax added the streams label May 29, 2020

vvcephei approved these changes May 30, 2020

View reviewed changes

abbccdda reviewed May 31, 2020

View reviewed changes

chia7712 reviewed May 31, 2020

View reviewed changes

abbccdda approved these changes Jun 3, 2020

View reviewed changes

mjsax added 6 commits June 3, 2020 16:25

KAFKA-10066: TestOutputTopic should pass record headers into

c8ec9ea

deserializers

Unify test

e58a0c4

foo

2f2515c

Fix checkstyle

ef08ac8

Github comment

6e3563f

Rename test

32ec931

mjsax force-pushed the kafka-10066-record-header-testoutputtopic branch from 4bbe972 to 32ec931 Compare June 3, 2020 23:38

mjsax merged commit 6383a22 into apache:trunk Jun 4, 2020

mjsax added a commit that referenced this pull request Jun 4, 2020

KAFKA-10066: TestOutputTopic should pass record headers into deserial…

382bb28

…izers (#8759) Reviewers: John Roesler <john@confluent.io>, Boyang Chen <boyang@confluent.io>, Chia-Ping Tsai <chia7712@gmail.com>

mjsax added a commit that referenced this pull request Jun 4, 2020

KAFKA-10066: TestOutputTopic should pass record headers into deserial…

00e265e

…izers (#8759) Reviewers: John Roesler <john@confluent.io>, Boyang Chen <boyang@confluent.io>, Chia-Ping Tsai <chia7712@gmail.com>

mjsax deleted the kafka-10066-record-header-testoutputtopic branch June 4, 2020 23:30

Uh oh!

Conversation

mjsax commented May 29, 2020

Uh oh!

vvcephei left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mjsax commented Jun 2, 2020

Uh oh!

mjsax commented Jun 2, 2020

Uh oh!

mjsax commented Jun 2, 2020

Uh oh!

mjsax commented Jun 2, 2020

Uh oh!

mjsax commented Jun 3, 2020

Uh oh!

mjsax commented Jun 4, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants