Make Serdes less confusing in Scala#4963
Conversation
1e6f092 to
f8dc871
Compare
|
This is ready to be reviewed. |
|
@guozhangwang would it be possible to have a look at this before any release? |
guozhangwang
left a comment
guozhangwang
left a comment
There was a problem hiding this comment.
@debasishg could you also take a look and see if it makes sense to you?
There was a problem hiding this comment.
These two window and session serdes are not included in the new class anymore: is this intentional?
There was a problem hiding this comment.
This is a mistake while porting the change from lightbend/kafka-streams-scala, I will add them back.
There was a problem hiding this comment.
Since we are moving around these classes, we'd also need to double check if any of the corresponding web docs in docs/streams need to be updated. I'd suggest making a pass over the doc changes in #4756 and see if any of them need to be updated accordingly.
For example, if we have in any places mentions about the wrapper serializer / deserializer / etc, we should remove them then.
There was a problem hiding this comment.
I was thinking about this indeed. I will have a check this evening and push any changes.
|
@guozhangwang I pushed the correction, let me know if there is anything else. |
guozhangwang
left a comment
guozhangwang
left a comment
There was a problem hiding this comment.
LGTM. Leave to @debasishg to make another pass.
|
Thanks @guozhangwang for your review. |
|
LGTM. |
|
I just pushed with a rebase to make the CI run again since I believe the failing test is a flaky one. |
|
Thanks @joan38 , merged to trunk! |
|
@guozhangwang @debasishg Thanks |
Serdes are confusing in the Scala wrapper: * We have wrappers around Serializer, Deserializer and Serde which are not very useful. * We have Serdes in 2 places org.apache.kafka.common.serialization.Serde and in DefaultSerdes, instead we should be having only one place where to find all the Serdes. I wanted to do this PR before the release as this is a breaking change. This shouldn't add more so the current tests should be enough. Reviewers: Debasish Ghosh <dghosh@acm.org>, Guozhang Wang <guozhang@confluent.io>
3 participants
This is the corresponding PR of lightbend/kafka-streams-scala#70
Serdes are confusing in the Scala wrapper:
Serializer,DeserializerandSerdewhich are not very useful.org.apache.kafka.common.serialization.Serdeand incom.lightbend.kafka.scala.streams.DefaultSerdes, instead we should be having only one place where to find all the Serdes.I wanted to do this PR before the release as this is a breaking change.
This shouldn't add more so the current tests should be enough.
Committer Checklist (excluded from commit message)