You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@camel.apache.org by GitBox <gi...@apache.org> on 2021/03/25 12:11:31 UTC
[GitHub] [camel] jenskordowski opened a new pull request #5263: CAMEL-16060 camel-kafka - decouple kafka.PARTITION_KEY from kafka.KEY
jenskordowski opened a new pull request #5263:
URL: https://github.com/apache/camel/pull/5263
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel] omarsmak commented on pull request #5263: CAMEL-16060 camel-kafka - decouple kafka.PARTITION_KEY from kafka.KEY
Posted by GitBox <gi...@apache.org>.
omarsmak commented on pull request #5263:
URL: https://github.com/apache/camel/pull/5263#issuecomment-806788723
> Hi @omarsmak,
> I wasn't able to execute the KafkaProducerFullTest for some reason with docker. Haven't had time to look into this. Is there anything special to consider? I checked the test quickly, it appears it already executes some calls without key (but with partition_key).
>
> 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
```
Explicitly specify the partition (only used if the KafkaConstants.KEY header is defined)
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel] jenskordowski commented on pull request #5263: CAMEL-16060 camel-kafka - decouple kafka.PARTITION_KEY from kafka.KEY
Posted by GitBox <gi...@apache.org>.
jenskordowski commented on pull request #5263:
URL: https://github.com/apache/camel/pull/5263#issuecomment-806803908
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.
I will follow up on this and the tests as well.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel] davsclaus merged pull request #5263: CAMEL-16060 camel-kafka - decouple kafka.PARTITION_KEY from kafka.KEY
Posted by GitBox <gi...@apache.org>.
davsclaus merged pull request #5263:
URL: https://github.com/apache/camel/pull/5263
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel] jenskordowski commented on pull request #5263: CAMEL-16060 camel-kafka - decouple kafka.PARTITION_KEY from kafka.KEY
Posted by GitBox <gi...@apache.org>.
jenskordowski commented on pull request #5263:
URL: https://github.com/apache/camel/pull/5263#issuecomment-808150528
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel] omarsmak commented on pull request #5263: CAMEL-16060 camel-kafka - decouple kafka.PARTITION_KEY from kafka.KEY
Posted by GitBox <gi...@apache.org>.
omarsmak commented on pull request #5263:
URL: https://github.com/apache/camel/pull/5263#issuecomment-806881525
> 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.
> I will follow up on this and the tests as well.
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 :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel] jenskordowski commented on pull request #5263: CAMEL-16060 camel-kafka - decouple kafka.PARTITION_KEY from kafka.KEY
Posted by GitBox <gi...@apache.org>.
jenskordowski commented on pull request #5263:
URL: https://github.com/apache/camel/pull/5263#issuecomment-806742203
Hi @omarsmak,
I wasn't able to execute the KafkaProducerFullTest for some reason with docker. Haven't had time to look into this. Is there anything special to consider? I checked the test quickly, it appears it already executes some calls without key (but with partition_key).
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel] omarsmak commented on pull request #5263: CAMEL-16060 camel-kafka - decouple kafka.PARTITION_KEY from kafka.KEY
Posted by GitBox <gi...@apache.org>.
omarsmak commented on pull request #5263:
URL: https://github.com/apache/camel/pull/5263#issuecomment-808188055
> 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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org