CAMEL-12665 - Apache Pulsar integration via Component - #2849
Conversation
oscerd
left a comment
There was a problem hiding this comment.
The component must be added to the kit, the component starter seems to be missing, we may need to add a Karaf feature to support this on OSGi container and we need integration tests for Karaf and Spring Boot.
First we need to clean up everything a bit, lot of TODOs and some unused classes or empty methods etc.
|
@davsclaus please have a look too :-) |
| import org.slf4j.LoggerFactory; | ||
|
|
||
| @Ignore //TODO use TestContainers to spin up local pulsar broker | ||
| public class PulsarConsumerInTest extends CamelTestSupport { |
There was a problem hiding this comment.
why not having, single test class. This is a fast review. I may not see the reason promptly
|
What's the status of this one? Once we have this finished, we need a new PR for master |
|
I'm going to spend some time doing the last changes today (against this branch), then open a new PR against master when this version is finished. |
|
This is now ready for another review. |
oscerd
left a comment
There was a problem hiding this comment.
Only that minor comment. Probably you need to run again the profile sourcecheck and fix Codestyle
|
btw we have a camel-testcontainers and camel-testcontainers-spring, so you can use them instead of using the explicit dependency. There are examples, like camel-nats or camel-consul |
|
Thanks |
|
Pushed a load of checkstyle changes |
|
We need to align the component code and example code in 2.x to what we have in master now. |
|
|
Yes, but something is not like camel 3, for example the META-INF stuff for the component is still to be explicitly added when on 2.x, while on 3.x is just an annotation that will do the trick... So it's not an easy operation to backport from master to 2.x |
… as it prevents mocking" This reverts commit c48c4df.
| private final PulsarClient pulsarClient; | ||
|
|
||
| PulsarComponent(CamelContext context, AutoConfiguration autoConfiguration, PulsarClient pulsarClient) { | ||
| super(context); |
There was a problem hiding this comment.
The constructor must be changed like on master.
|
Is the I am asking as the endpoint uri syntax is a bit wrong and we need to tidy this up a bit. For example we may want to have an enum with those 2 values so Camel knows what you can specify which also allows tooling to know that. And do you need that double slash in the topic name? eg can this become |
We can improve it for sure. On master I had to change it to persistence in uri:syntax because it breaks the adoc rendering |
|
Also we need to make the unit tests run when there is no docker environment, so you dont get failures. |
In Pulsar a topic is either persistent or non-persistent. It must include the double forward slashes after the colon when passed to pulsar i.e |
|
what's the status of this one? |
Have aligned the changes done in master. |
|
I'm working on the merge. @foamdino and @rshermanTHG can you please improve the documentation under src/main/docs on master? |
|
The build is failing. Can you please check on 2.x? |
|
@davsclaus can you take a look? |
|
@oscerd I think I fixed something similar on master branch in that SB generator. |
|
okay building 2.x now |
|
Okay you need to be able to build the component without having docker running, so that should be fixed too. |
|
Okay so the src/main/resources/META-INF/services/org/apache/camel/component/pulsar file has log4j settings, instead it should be |
|
@oscerd you can fix it as above ^^^ |
|
Thank you |
|
I'll backport the Karaf feature and tests for Karaf and SB too |
|
Backported. Thanks. Please @rshermanTHG and @foamdino take a look at the documentation under src/main/docs and improve it if you have time. |
Contains new camel-pulsar component and example usage camel-example-spring-pulsar.
Looking forward to feedback on design/style code quality.