Skip to content
This repository was archived by the owner on May 25, 2023. It is now read-only.

Conversation

@debasishg
Copy link
Contributor

@debasishg debasishg commented Feb 11, 2018

This PR offers a sample implementation of providing implicit serialization / de-serialization in Kafka Streams APIs (Issue #46).

This may not be the final implementation but provides a groundwork for further discussion. It will be great if we can have alternate implementations that make the APIs more compile time safe. An approach has been suggested by @dhoepelman in #46. But I could not make the implicit lookup work properly and it became a mess of implicits.

Design decisions adopted in the implementation :

  1. The implementation offers implicits to provide the serializers and de-serializers but at the same time also allows the opt-in to use the default serializers registered in the Kafka Streams config.
  2. The optional implicit pattern is implemented with the usual null-default-value trick, but with a difference. The technique used is adopted from this blog post.
  3. The standard way to implement the null-default-value trick could not be applied as Scala does not allow a mix of default values and function overloads. And we have quite a few examples of such overloaded functions in the Kafka Streams API set.
  4. The implementation allows implicits for the Serdes or for Serialized, Consumed and Produced. The test examples demonstrate both, though the implicits for Serdes make a cleaner implementation.
  5. The implementation does a trade-off in using the null-default-value trick as it moves some of the compile time errors to runtime.

Examples:

  1. The example StreamToTableJoinScalaIntegrationTestImplicitSerdes demonstrates how to use the technique of implicit Serdes
  2. The example StreamToTableJoinScalaIntegrationTestImplicitSerialized demonstrates how to use the technique of implicit Serialized, Consumed and Produced.
Copy link

@blublinsky blublinsky left a comment

Choose a reason for hiding this comment

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

LGTM,
very nice

Copy link

@deanwampler deanwampler left a comment

Choose a reason for hiding this comment

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

Added one question about testing the usage patterns. Otherwise, LGTM


def groupBy[KR](selector: (K, V) => KR, serialized: Serialized[KR, V]): KGroupedStreamS[KR, V] = {
inner.groupBy(selector.asKeyValueMapper, serialized)
/**

Choose a reason for hiding this comment

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

I'm not sure if I saw this, but do the contests explicitly confirm all three usage patterns. Specifically, I'm not sure I saw that #1 was being tested.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are correct! I added that test ..

Choose a reason for hiding this comment

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

Great!

Copy link

@seglo seglo left a comment

Choose a reason for hiding this comment

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

I love the Perhaps pattern. LGTM. 👍


}
// technique for optional implicits adopted from
// http://missingfaktor.blogspot.in/2013/12/optional-implicit-trick-in-scala.html
Copy link

Choose a reason for hiding this comment

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

Awesome find! 👍

@debasishg debasishg merged commit 5b26552 into develop Feb 14, 2018
@debasishg debasishg deleted the serdes-fold branch March 13, 2018 17:01
// technique for optional implicits adopted from
// http://missingfaktor.blogspot.in/2013/12/optional-implicit-trick-in-scala.html

case class Perhaps[E](value: Option[E]) {

Choose a reason for hiding this comment

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

It probably should be made final and extend AnyVal to reduce memory allocation and improve inlining.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah true, but in the latest release we have got rid of this abstraction altogether.

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

Labels

None yet

6 participants