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

chore: Add header deserialize option on Kafka source #842

Merged
merged 2 commits into from Mar 29, 2022

Conversation

christophd
Copy link
Contributor

  • Adds utility class to auto deserialize message headers from byte[] to String
  • Option must be explicitly enabled on the source Kamelet
  • Add some automated tests on Kafka source/sink Kamelets

This PR uses a new utility class org.apache.camel.kamelets.utils.serialization.kafka.KafkaHeaderDeserializer in kafka-source.kamelet. The CI tests on this PR are expected to fail because of this until the utility class is available in main_SNAPSHOT.

Please have a look at the PR on my fork to see the successful tests CI workflow: christophd#4

@oscerd
Copy link
Contributor

oscerd commented Mar 18, 2022

Cc @lburgazzoli

}

for (Map.Entry<String, Object> header : headers.entrySet()) {
header.setValue(typeConverter.convertTo(String.class, header.getValue()));
Copy link
Contributor

Choose a reason for hiding this comment

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

there are a number of headers that the kafka component adds i.e. kafka.OFFSET and other things like that, I guess those should not be converted automatically

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have excluded kafka.HEADERS and CamelKafkaManualCommit by default now as these are not trivial to deserialize to String. As a user I would expect other headers to be deserialized by default

@lburgazzoli
Copy link
Contributor

Cc @lburgazzoli

Added a few comments :)

- Adds utility class to auto deserialize message headers from byte[] to String
- Option must be explicitly enabled on the source Kamelet
- Exclude non String Kafka headers from deserialization (kafka.HEADERS and CamelKafkaManualCommit)
@christophd christophd force-pushed the pr/kafka-header-deserializer branch 4 times, most recently from d879a96 to b6125bf Compare March 21, 2022 20:40
@christophd
Copy link
Contributor Author

@lburgazzoli I am happy now with this. Mind having another look?

@lburgazzoli
Copy link
Contributor

@christophd LGTM, I would go a second round of review around the conversion of the headers added bu the component, i.e. all those with a kafka. prefix as changing to a string may cause some behavioral changes i.e. if you are using a simple expression to do some manipulation.

@oscerd
Copy link
Contributor

oscerd commented Mar 28, 2022

Is there something more to add?

@lburgazzoli
Copy link
Contributor

@oscerd @christophd is is fine with me to merge

Points to further investigation after merge could be:

  1. move the KafkaHeaderDeserializer to the camel-kafka component (guess it could be useful there too)
  2. reason about excluding the headers added by the component from the headers to be converted to string

@oscerd
Copy link
Contributor

oscerd commented Mar 29, 2022

Thanks.

@oscerd oscerd merged commit 7c959e1 into apache:main Mar 29, 2022
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

3 participants