Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

KAFKA-8326: Introduce List Serde #6592

Merged
merged 49 commits into from
May 13, 2021
Merged

KAFKA-8326: Introduce List Serde #6592

merged 49 commits into from
May 13, 2021

Conversation

yeralin
Copy link
Contributor

@yeralin yeralin commented Apr 17, 2019 •
edited by mjsax
Loading

Introduce List serde for primitive types or custom serdes with a serializer and a deserializer according to https://cwiki.apache.org/confluence/display/KAFKA/KIP-466:+Add+support+for+List%3CT%3E+serialization+and+deserialization

Test cases for the new serde will be added once the PR is reviewed by repo maintainers.

@yeralin
Copy link
Contributor Author

yeralin commented Apr 17, 2019 •
edited
Loading

Usage: new ListSerde<>(Serdes.String()) to create a serde for List<String> values

@yeralin yeralin changed the title Introduce list serde [MINOR] Introduce list serde Apr 22, 2019
@yeralin
Copy link
Contributor Author

yeralin commented Apr 22, 2019

@mjsax

@mjsax
Copy link
Member

mjsax commented Apr 22, 2019

Thanks a lot for the PR @yeralin! I like the idea and think it's a good addition.

However, adding those classes is a public API change and not a MINOR PR. Thus, it should be backed by a JIRA ticket and it also requires a KIP: https://cwiki.apache.org/confluence/display/KAFKA/Kafka+Improvement+Proposals

Let me know if you have any question about the KIP process (should actually be well document in the wiki), or if you need any other assistance.

Btw: You should also add unit tests :)

@yeralin yeralin changed the title [MINOR] Introduce list serde [KIP-466] Introduce list serde May 6, 2019
@mjsax
Copy link
Member

mjsax commented May 6, 2019

Great to see some progress on this PR, but we still need a KIP... I would recommend to work on the KIP first (or in parallel), @yeralin

@yeralin
Copy link
Contributor Author

yeralin commented May 6, 2019

@mjsax KIP, JIRA, and DISCUSS thread are started: https://cwiki.apache.org/confluence/display/KAFKA/KIP-466%3A+Add+support+for+List%3CT%3E+serialization+and+deserialization

@yeralin
Copy link
Contributor Author

yeralin commented May 13, 2019

Hmmm, strange I added a test case, but it still says that test results were not found

@mjsax mjsax changed the title [KIP-466] Introduce list serde KAFKA-8326: Introduce List Serde May 24, 2019
mjsax
mjsax reviewed May 24, 2019
View reviewed changes
Copy link
Member

@mjsax mjsax left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some nits. I also replied to the KIP discuss thread.

clients/src/main/java/org/apache/kafka/common/serialization/ListDeserializer.java Outdated Show resolved Hide resolved
clients/src/main/java/org/apache/kafka/common/serialization/ListDeserializer.java Outdated Show resolved Hide resolved
clients/src/main/java/org/apache/kafka/common/serialization/ListDeserializer.java Show resolved Hide resolved
clients/src/main/java/org/apache/kafka/common/serialization/ListSerializer.java Outdated Show resolved Hide resolved
@mjsax mjsax added the streams label Jun 21, 2019
mjsax
mjsax reviewed Jun 22, 2019
View reviewed changes
Copy link
Member

@mjsax mjsax left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some initial comments.

clients/src/main/java/org/apache/kafka/common/serialization/ListDeserializer.java Outdated Show resolved Hide resolved
clients/src/main/java/org/apache/kafka/common/serialization/ListDeserializer.java Outdated Show resolved Hide resolved
clients/src/test/java/org/apache/kafka/common/serialization/SerializationTest.java Show resolved Hide resolved
mjsax
mjsax reviewed Jul 24, 2019
View reviewed changes
Copy link
Member

@mjsax mjsax left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some more comments.

I think we also need tests for the new configuration parameters to make sure they work correctly.

streams/src/main/java/org/apache/kafka/streams/StreamsConfig.java Outdated Show resolved Hide resolved
clients/src/main/java/org/apache/kafka/clients/CommonClientConfigs.java Outdated Show resolved Hide resolved
clients/src/main/java/org/apache/kafka/clients/CommonClientConfigs.java Outdated Show resolved Hide resolved
clients/src/main/java/org/apache/kafka/clients/consumer/ConsumerConfig.java Outdated Show resolved Hide resolved
clients/src/main/java/org/apache/kafka/clients/consumer/ConsumerConfig.java Outdated Show resolved Hide resolved
clients/src/main/java/org/apache/kafka/common/serialization/ListDeserializer.java Outdated Show resolved Hide resolved
clients/src/main/java/org/apache/kafka/common/serialization/ListDeserializer.java Outdated Show resolved Hide resolved
clients/src/main/java/org/apache/kafka/common/serialization/ListSerializer.java Outdated Show resolved Hide resolved
clients/src/main/java/org/apache/kafka/common/serialization/ListSerializer.java Outdated Show resolved Hide resolved
clients/src/main/java/org/apache/kafka/common/serialization/ListSerializer.java Outdated Show resolved Hide resolved
@yeralin
Copy link
Contributor Author

yeralin commented Jul 31, 2019

Any updates on this?

mjsax
mjsax reviewed Jul 31, 2019
View reviewed changes
Copy link
Member

@mjsax mjsax left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems you did not push an update yet. Stopped reviewing when I realized it.

The PR also need to be rebased to resolve conflict. Can review again, after it's updated.

clients/src/main/java/org/apache/kafka/clients/CommonClientConfigs.java Show resolved Hide resolved
clients/src/main/java/org/apache/kafka/clients/consumer/ConsumerConfig.java Outdated Show resolved Hide resolved
clients/src/main/java/org/apache/kafka/clients/consumer/ConsumerConfig.java Outdated Show resolved Hide resolved
streams/src/main/java/org/apache/kafka/streams/StreamsConfig.java Outdated Show resolved Hide resolved
clients/src/main/java/org/apache/kafka/common/serialization/ListDeserializer.java Outdated Show resolved Hide resolved
@yeralin
Copy link
Contributor Author

yeralin commented Jul 31, 2019 •
edited
Loading

@mjsax Sorry, I thought to finish all the discussions first, that's why I did not push.
New commit is in place! 😄

UPD: Merged with apache:trunk

mjsax
mjsax reviewed Aug 4, 2019
View reviewed changes
Copy link
Member

@mjsax mjsax left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stopped reviewing at this point -- there is too much noise in this PR. Avoid to change unrelated files -- it makes reviewing tedious. Please revert all changes to unrelated files (feel free to do a separate cleanup PR if you wish, but don't piggy-back random cleanups)

clients/src/main/java/org/apache/kafka/clients/CommonClientConfigs.java Show resolved Hide resolved
clients/src/main/java/org/apache/kafka/clients/admin/Admin.java Outdated Show resolved Hide resolved
clients/src/main/java/org/apache/kafka/clients/admin/AlterReplicaLogDirsOptions.java Outdated Show resolved Hide resolved
clients/src/main/java/org/apache/kafka/clients/consumer/ConsumerConfig.java Outdated Show resolved Hide resolved
clients/src/main/java/org/apache/kafka/clients/consumer/ConsumerConfig.java Outdated Show resolved Hide resolved
clients/src/main/java/org/apache/kafka/clients/producer/ProducerConfig.java Outdated Show resolved Hide resolved
@yeralin
Copy link
Contributor Author

yeralin commented Aug 5, 2019 •
edited
Loading

Hey, I'm sorry I thought you wanted me to merge with trunk that's why so many files popped up.

I think I know how to resolve this. I'll force push before the merge, and then keep pushing my commits only related to introduce_list_serde work. And at the end, once all reviews are completed, I'll do the final merge and we can close this PR.

UPD: Ok @mjsax it should be much cleaner now :)

@yeralin yeralin force-pushed the introduce_list_serde branch from 70d382b to b4e5449 Compare May 12, 2021 16:56
@yeralin
Copy link
Contributor Author

yeralin commented May 12, 2021

Ok, updated the KIP with serializing nulls for different strategies.

@yeralin yeralin force-pushed the introduce_list_serde branch from b4e5449 to e7c7789 Compare May 12, 2021 18:48
ableegoldman
ableegoldman reviewed May 12, 2021
View reviewed changes
Copy link
Contributor

@ableegoldman ableegoldman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good, I think once you just move the upgrade guide docs to the 3.0 section then we can merge.

docs/streams/upgrade-guide.html Outdated Show resolved Hide resolved
ableegoldman
ableegoldman approved these changes May 12, 2021
View reviewed changes
Copy link
Contributor

@ableegoldman ableegoldman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Just give me a ping when the build completes with tests passing (your tests, at least -- there will almost certainly be some unrelated failures, as a handful of tests are super flaky at the moment)

Review changes
37569c9 
Reword statements

Simplify deserialization logic
@yeralin yeralin force-pushed the introduce_list_serde branch from f3efeb2 to 37569c9 Compare May 13, 2021 20:14
@yeralin
Copy link
Contributor Author

yeralin commented May 13, 2021

@ableegoldman Seems like all checks passed :)

@ableegoldman
Copy link
Contributor

Build has only some unrelated test failures in known flaky KTableKTableForeignKeyInnerJoinMultiIntegrationTest#shouldInnerJoinMultiPartitionQueryable and
RaftClusterTest

@ableegoldman ableegoldman merged commit 6d1ae8b into apache:trunk May 13, 2021
@ableegoldman
Copy link
Contributor

Merged to trunk! Thanks @yeralin for all the work and patience it took to get this PR in, and to all the reviewers who helped get it here along the way.

@yeralin, can you update the KIP to note that it's completed, and then move this KIP to the "Adopted" section on both the KIP main page and the Streams KIPs subpage? 🙏

@mjsax
Copy link
Member

mjsax commented May 14, 2021

Thanks @yeralin for pushing it through and thanks @ableegoldman for helping out reviewing! Really nice addition!

@yeralin yeralin deleted the introduce_list_serde branch May 14, 2021 13:36
ijuma added a commit to ijuma/kafka that referenced this pull request May 16, 2021
@ijuma
Merge remote-tracking branch 'apache-github/trunk' into reduce-produc…
00c29d0 
…e-allocations-lz4

* apache-github/trunk: (155 commits)
  KAFKA-12728: Upgrade gradle to 7.0.2 and shadow to 7.0.0 (apache#10606)
  KAFKA-12778: Fix QuorumController request timeouts and electLeaders (apache#10688)
  KAFKA-12754: Improve endOffsets for TaskMetadata (apache#10634)
  Rework on KAFKA-3968: fsync the parent directory of a segment file when the file is created (apache#10680)
  MINOR: set replication.factor to 1 to make StreamsBrokerCompatibilityService work with old broker (apache#10673)
  MINOR: prevent cleanup() from being called while Streams is still shutting down (apache#10666)
  KAFKA-8326: Introduce List Serde (apache#6592)
  KAFKA-12697: Add Global Topic and Partition count metrics to the Quorum Controller (apache#10679)
  KAFKA-12648: MINOR - Add TopologyMetadata.Subtopology class for subtopology metadata (apache#10676)
  MINOR: Update jacoco to 0.8.7 for JDK 16 support (apache#10654)
  MINOR: exclude all `src/generated` and `src/generated-test` (apache#10671)
  KAFKA-12772: Move all transaction state transition rules into their states (apache#10667)
  KAFKA-12758 Added `server-common` module to have server side common classes.  (apache#10638)
  MINOR Removed copying storage libraries specifically as they are already copied. (apache#10647)
  KAFKA-5876: KIP-216 Part 4, Apply InvalidStateStorePartitionException for Interactive Queries (apache#10657)
  KAFKA-12747: Fix flakiness in shouldReturnUUIDsWithStringPrefix (apache#10643)
  MINOR: remove unnecessary placeholder from WorkerSourceTask#recordSent (apache#10659)
  MINOR: Remove unused `scalatest` definition from `dependencies.gradle` (apache#10655)
  MINOR: checkstyle version upgrade: 8.20 -> 8.36.2 (apache#10656)
  KAFKA-12464: minor code cleanup and additional logging in constrained sticky assignment (apache#10645)
  ...
@venkatesh010
Copy link

Hey @yeralin @mjsax @ableegoldman Getting SerializationException in this serde
SerializationException: Invalid serialization strategy flag value

Flag value is derived from bytes size (which is coming >2) which is amount of enum variables hence its breaking
seems to be a bug

please check

@venkatesh010
Copy link

This is while using List where E is class which is Inner
Serde used for Inner is JsonSerde of Type E

@yeralin
Copy link
Contributor Author

yeralin commented Jun 23, 2023

@venkatesh010 could you please provide a code snippet?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JakobEdding JakobEdding JakobEdding left review comments

@abbccdda abbccdda abbccdda left review comments

@zorgz zorgz zorgz left review comments

@miguno miguno miguno left review comments

@wcarlson5 wcarlson5 wcarlson5 approved these changes

@ableegoldman ableegoldman ableegoldman approved these changes

@vvcephei vvcephei Awaiting requested review from vvcephei

@mjsax mjsax Awaiting requested review from mjsax

Assignees
No one assigned
Labels
kip Requires or implements a KIP streams
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.