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

Correct kafka prefix #1284

Closed
wants to merge 1 commit into from
Closed

Conversation

clyang82
Copy link

Fixes #

Proposed Changes

Release Note


Signed-off-by: clyang82 <[email protected]>
@clemensv
Copy link
Contributor

This is a breaking spec change.

If the code doesn't align with the spec, the code is wrong, not the spec.

@duglin
Copy link
Collaborator

duglin commented May 21, 2024

It looks like this is broken for just the Confluent Kafka stuff in the go-sdk, the older Sarama uses ce_, @clyang82 perhaps open a PR in the go sdk?

@duglin
Copy link
Collaborator

duglin commented May 21, 2024

@embano1 beat you to it! :-) cloudevents/sdk-go#1059
thanks @embano1

Going to close this PR

@duglin duglin closed this May 21, 2024
@clyang82 clyang82 deleted the correct_prefix branch May 23, 2024 01:48
@clyang82
Copy link
Author

Thanks @embano1 @duglin @clemensv

Could you also share the reason or background that why different spec has different behaviours?

  • HTTP is using ce-
  • MQTT is using ce-
  • PubSub is using ce-
  • Kafka is using ce_

@embano1
Copy link
Member

embano1 commented May 23, 2024

At least for Kafka, I can't see a specific reason (in the Kafka APIs) why it must be _ instead of -.

@clyang82
Copy link
Author

If there is no specific reason, could we keep consistent with others? I know it will be a breaking spec change. can we do it in a new version? Thanks.

@embano1
Copy link
Member

embano1 commented May 27, 2024

If there is no specific reason, could we keep consistent with others? I know it will be a breaking spec change. can we do it in a new version? Thanks.

Will be introduced in the upcoming sdk-go version highlighted as breaking change. Not sure if we have this implementation (Confluent) in other SDKs, guess Go was the first one to introduce.

@clyang82
Copy link
Author

Thanks @embano1. I think we only have confluent implementation in sdk-go. Correct me if I am wrong @yanmxa
But I think the breaking change will be in [sarama](https://github.com/cloudevents/sdk-go/tree/main/protocol/kafka_sarama) as well.

Since you agree to use ce- to keep consistent with others, does it make sense to support ce- in confluent firstly (It is just introduced in this release)?

@embano1
Copy link
Member

embano1 commented May 29, 2024

Correct me if I am wrong @yanmxa But I think the breaking change will be in [sarama](https://github.com/cloudevents/sdk-go/tree/main/protocol/kafka_sarama) as well.

No, sarama implementation follows the Kafka binding spec which defines ce_ semantics.

Since you agree to use ce- to keep consistent with others

I agreed that there doesn't seem to be a specific need why it must be _ instead of - in Kafka. However, since the binding spec defines it as such, we can't change it without breaking all (sarama) users. Besides the different naming styles, is there a concern with using ce_ as per the Kafka binding spec?

@clyang82
Copy link
Author

I am fine with ce_ for kafka now. But I am thinking that we should expose the cloudevent attribute prefix in sdk so that the user can use it directly instead of define a variable again in their code. if so, keep the prefix with the same naming styles should be considered.

@embano1
Copy link
Member

embano1 commented May 30, 2024

But I am thinking that we should expose the cloudevent attribute prefix in sdk so that the user can use it directly instead of define a variable again in their code

I'm not quite following why this would be needed/useful for the client because those prefixes/headers are transport related. Can you please elaborate?

@clyang82
Copy link
Author

Yes. those prefixed are transport related. but we do not have reasons to have different attribute prefix. If we have the same attribute prefix for all transport, put it into a constants to use by internal and external. just my two cents. Thanks.

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.

4 participants