Skip to content

MINOR: Extended Scala serdes implicit conversions with optional custom naming #6245

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.

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

Closed

Conversation

mowczare
Copy link
Contributor

@mowczare mowczare commented Feb 8, 2019

Since custom state store and repartition topic naming in DSL API is now possible in Kafka 2.1.0, this proposed feature enhance implicit conversions for the Scala API making it possible to call (assuming val stream: KGroupedStream[K, V]:

stream.aggregate(initializer)(aggregateFunc)("customNameTopic")

instead of:
stream.aggregate(initializer)(aggregateFunc)(Materialized.as[K, V, ByteArrayWindowStore]("customStateStoreName")

This change is backward compatible, that said: stream.aggregate(initializer)(aggregateFunc) also works with default naming convention.

These implicit conversions are propagated for Materialized, Grouped and Joined as well.

@mowczare
Copy link
Contributor Author

mowczare commented Feb 8, 2019

summon @guozhangwang @debasishg @joan38 @bbejeck for review.

@mowczare mowczare changed the title Extended Scala serdes implicit conversions with optional custom naming MINOR: Extended Scala serdes implicit conversions with optional custom naming Feb 11, 2019
*/
* Implicit conversions between the Scala wrapper objects and the underlying Java
* objects.
*/
Copy link
Contributor

@joan38 joan38 Feb 11, 2019

Choose a reason for hiding this comment

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

I like this still because it's the default in Scala but it seems that in this code base we adopted the Java style.
So for consistency I guess we should leave it to Java style or bigbang the whole codebase to Scala style.

You need to use gradle spotlessApply and commit the formatting change, then your Jenkins should pass.

Grouped.`with`[K, V]
}
Copy link
Contributor

Choose a reason for hiding this comment

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

These are redundant braces we don't need to add them.

gradle spotlessApply will solve it all for you.

@joan38
Copy link
Contributor

joan38 commented Feb 11, 2019

This PR is going against the Scala spirit of type-safety.
This makes use of implicit conversion that the compiler team is trying hard to get rid of it (see scala/scala3#2060)

We should avoid as much as possible to use "primitive" types like Strings and use actual meaningful types:

stream.aggregate(initializer)(aggregateFunc)(Materialized.as("customStateStoreName"))

has a good compile time safety compare to:

stream.aggregate(initializer)(aggregateFunc)("customStateStoreName")

and even worst:

val materializerStore = "customStateStoreName"
stream.aggregate(initializer)(aggregateFunc)(materializerStore)

@mowczare mowczare closed this Feb 12, 2019
@joan38
Copy link
Contributor

joan38 commented Feb 12, 2019

@mowczare hope this is not discouraging!
Thanks for the PR.
Also please don't hesitate to ask more questions on this! I'm always happy to provide more context.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants