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

Should KafkaProducerMessage & KafkaConsumerMessage extend AbstractMessage? #64

Open
antonkomarev opened this issue Jun 2, 2021 · 7 comments

Comments

@antonkomarev
Copy link

antonkomarev commented Jun 2, 2021

What is the reason of extending both of KafkaProducerMessage and KafkaConsumerMessage from the AbstractKafkaMessage?

I might be wrong, but it looks like we cannot make $key and $body parameters of the KafkaConsumerMessage::__construct method strictly typed because we don't know what type of value could be passed to KafkaProducerMessage, but in KafkaConsumerMessage we are always getting a strings.

@nick-zh
Copy link
Contributor

nick-zh commented Jun 3, 2021

Heya, can you elaborate a bit more, sry i am not sure that i am following. key and body can be null
What part are you referring to, concerning in KafkaConsumerMessage we are always getting strings?

@antonkomarev
Copy link
Author

antonkomarev commented Jun 3, 2021

I mean right now they are defined as mixed in docblocks and does not have types in method signature:

public function __construct(
    string $topicName,
    int $partition,
    int $offset,
    int $timestamp,
    $key,
    $body,
    ?array $headers
) {

I thought both of them should be ?string, am I wrong?

@nick-zh
Copy link
Contributor

nick-zh commented Jun 3, 2021

body for sure is mixed, it can be anything (array , string, null, etc.)
key in PHP can only be ?string but in other languages other primitive types can also be used
It would fair to change the key to ?string but mixed keeps it open, if the extension would ever support the other types as well

@antonkomarev
Copy link
Author

antonkomarev commented Jun 3, 2021

Aren't they all persisted as nullable string in Kafka?

@nick-zh
Copy link
Contributor

nick-zh commented Jun 3, 2021

Only in the PHP implementation (at least from what i know), in Java you are able to send an int and other types as well.

@nick-zh
Copy link
Contributor

nick-zh commented Jun 3, 2021

Even if it is handled as string internally, you can have an avro encoded key, which can have any structure (int, string, array, etc.) at least from what i know. I haven't used anything else than string (avro and non-avro) so far 😄
If you have other insights, i am happy to discuss changes we can do ✌️

@nick-zh
Copy link
Contributor

nick-zh commented Jun 3, 2021

Again thx for the input, i quickly tested this out and yeah i can send an array (avro) as key, so we should open even more and make withKey for the producer accept mixed instead of ?string (issue created #65)

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

No branches or pull requests

2 participants