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.

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

added new optional param classInstance to Serializer and Deserializer helper classes #5

Merged
merged 2 commits into from
Jan 19, 2017

Conversation

kdrakon
Copy link
Contributor

@kdrakon kdrakon commented Jan 16, 2017

These are defaulted to perform the normal behaviour: reflective construction of the specified classes. The param can then be set so that one can specify (provide) logic that defines how an instance of the class should be constructed. The primary use case for this was special serializers that take constructor parameters.

Addresses Issue #4

…lizer` helper classes. These are defaulted to perform the normal behaviour: reflective construction of the specified classes. The param can then be set so that one can specify (provide) logic that defines how an instance of the class should be constructed. The primary use case for this was special serializers that take constructor parameters.
@kdrakon
Copy link
Contributor Author

kdrakon commented Jan 17, 2017

As an example of the usecase using Confluents Avro deserializer:

import io.confluent.kafka.schemaregistry.client.CachedSchemaRegistryClient
import io.confluent.kafka.serializers.KafkaAvroDeserializer

val schemaRegistryClient = new CachedSchemaRegistryClient("http://localhost:8081", 42)

implicit val avroDeserializer =
  Deserializer[AnyRef](
    "io.confluent.kafka.serializers.KafkaAvroDeserializer",
    classOf[KafkaAvroDeserializer],
    (d) => new KafkaAvroDeserializer(schemaRegistryClient)
  )

Copy link
Member

@alexandru alexandru left a comment

Choose a reason for hiding this comment

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

Looks good, I just have this little obsession against verbose naming and if you could change ClassInstanceProvider to Constructor, that would be great.

Thanks for the PR.

@@ -27,14 +27,18 @@ import language.existentials
*/
final case class Serializer[A](
className: String,
classType: Class[_ <: KafkaSerializer[A]]) {
classType: Class[_ <: KafkaSerializer[A]],
classInstance: Serializer.ClassInstanceProvider[A] = (s: Serializer[A]) => s.classType.newInstance()) {
Copy link
Member

Choose a reason for hiding this comment

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

Default parameter looks ugly (personal pet peeve). I'm fairly certain that Scala accepts this:

classInstance: Serializer.ClassInstanceProvider[A] = _.classType.newInstance()

While we are at it classInstance: Serializer.ClassInstanceProvider is a little redundant and verbose. So lets simplify to:

constructor: Serializer.Constructor[A] = _.classType.newInstance()

// ... 

object Serializer {
  type Constructor[A] = (Serializer[A]) => KafkaSerializer[A]
  // ...

Naming, right? :-) I don't care that much, so it's up to you.

@alexandru
Copy link
Member

@kdrakon can I bother you with also adding a comment for that new type alias and for the params of the Serializer and Deserializer classes? The config is has a good Scaladoc, but not these.

*/
final case class Deserializer[A](
className: String,
classType: Class[_ <: KafkaDeserializer[A]]) {
classType: Class[_ <: KafkaDeserializer[A]],
constructor: Deserializer.Constructor[A] = (d: Deserializer[A]) => d.classType.newInstance()) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@alexandru unfortunately, I couldn't simplify this to _.classType.newInstance(). There is a

missing parameter type for expanded function

warning, but because -Xfatal-warnings is enabled, this makes it impossible to compile.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Other than that, the rest of the suggestions have been pushed to the PR.

@alexandru
Copy link
Member

Thanks @kdrakon, I'm merging it, I hope you tested it :-)

@alexandru alexandru merged commit 970cb86 into monix:master Jan 19, 2017
@kdrakon
Copy link
Contributor Author

kdrakon commented Jan 19, 2017

Ran the tests locally a few times, but tested it more with my localised project 😅

@Avasil Avasil mentioned this pull request Sep 14, 2019
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.

None yet

2 participants