-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
CAMEL-16060 camel-kafka - decouple kafka.PARTITION_KEY from kafka.KEY #5263
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, we need to add a note to the migration guide for 3.10
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @jenskordowski for the PR! Although this looks good to me, the PR breaks the KafkaProducerFullTest
, can you please take a look at it?
Also, can you please add a test case in the KafkaProducerFullTest
where you only send a message with only KafkaConstants.PARTITION_KEY
without KafkaConstants.KEY
being set just to make sure nothing else break.
Also, the documentation it needs to be updated to reflect this change here whereby the hard limitation is no longer needed.
Hi @omarsmak, AFAIK the documentation does not cover the fact that both headers are required to define the partition (today). That confused me at first and I checked the code to find that out. |
Here is a snippet for the falling tests https://sharetext.me/raw/xq55wisjrn. Would be great if you can take a look at that to see why is that failing as these FullTests of camel-kafka are very important and they need to pass with no issues. in regards to the documentation, it is actually mentioned there: https://github.com/apache/camel/blob/master/components/camel-kafka/src/main/docs/kafka-component.adoc#producer-headers
|
Interesting, the "customer"-facing page appears to have a rendering-issue with the table. At least I cannot see the last cell that contains this info. |
Yes you are right, it wasn't visible due to some formatting issues in the adoc. I fixed that earlier and thus is visible again :) |
Yes, I also noticed when updating the documentation. I updated the tests. Many tests actually "tried" to write into a certain partition, but this did not work before / was ignored (as no kafka.KEY was set). As the topic is auto-created in the tests, it contains a single partition only, which is 0 and not 1. I changed that now and the tests pass. There is a single test that does not run successfully (KafkaConsumerManualCommitTest). But this one fails for me also before my change and is unrelated anyway. Feel free to squash those two changes into one when merging. Not sure, how you handle this in similar cases. |
It is looking good to me! Thanks a lot. One more thing, can you please fix the conflicts in the documentation and then we are good to merge this PR |
PR for https://issues.apache.org/jira/browse/CAMEL-16060
I removed the if-else block altogether, as each field may be null during ProducerRecord instantiation. I also noticed that the original / not-converted key was passed to the constructor, which looks like a bug to me. I changed it accordingly.