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

[WIP][rdkafka] Introduce KeySerializer #257

Closed
wants to merge 10 commits into from

Conversation

tPl0ch
Copy link
Contributor

@tPl0ch tPl0ch commented Nov 6, 2017

This is a WIP prototype to introduce the concept of key serialization as discussed in #255 and #254.

The following steps will be taken in order to implement the feature:

  • Define KeySerializer interface
  • Implement default NoOpKeySerializer incl. tests
  • Implement JsonKeySerializer incl. tests
  • Add KeySerializerAwareTrait
  • Integrate key serialization into RdKafkaProducer incl. tests
  • Integrate key de-serialization into RdKafkaConsumer incl. tests
  • Extract json_* functions

@makasim
Copy link
Member

makasim commented Nov 6, 2017

It has just occurred to me that the key could be serialized before setting it to the message (outside of the producer). Would it be better and simpler to go that way?

@tPl0ch
Copy link
Contributor Author

tPl0ch commented Nov 6, 2017

I would be confused if the serialization code for messages and keys was in different parts. You could also use the same argument for moving the serialization of the body/value of the message. Also it might be that you want to have different Serializers for different Producers.

*/
public function toString($key)
{
return $key;
Copy link
Member

Choose a reason for hiding this comment

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

maybe we should force casting to a string here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That would not be a NoOp anymore IMO, since casting would actually change they type. I would rather just keep the current behaviour and just pass the key through by default. What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

Than it contradicts to the interface contract. The KeySerializer toString methods returns string and I as a client should be able to rely on it.

Also the rdkafka producer topic declares a key as a string

https://arnaud-lb.github.io/php-rdkafka/phpdoc/class.rdkafka-producertopic.html

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, I agree. But then I'd rather rename it to DefaultSerializer to make it clear that there might be a type conversion.

Copy link
Member

Choose a reason for hiding this comment

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

OK, I agree. But then I'd rather rename it to DefaultSerializer to make it clear that there might be a type conversion.

Sounds good.

@tPl0ch
Copy link
Contributor Author

tPl0ch commented Nov 6, 2017

@makasim I just had a different Idea actually. How about using a SerializingMessageDecorator? This way we could actually reduce the complexity in the Producer/Consumer classes and have a clean SRP inside the Decorator. By default, we could create a Decorator with JsonSerializer and DefaultKeySerializer.

@makasim
Copy link
Member

makasim commented Nov 6, 2017

@tPl0ch one class that implements two interfaces?

@makasim makasim closed this Mar 10, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants