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

CAMEL-13807: Add Component DSL fluent builders #3521

Merged
merged 42 commits into from Feb 4, 2020
Merged

CAMEL-13807: Add Component DSL fluent builders #3521

merged 42 commits into from Feb 4, 2020

Conversation

omarsmak
Copy link
Member

@omarsmak omarsmak commented Jan 27, 2020

This PR implements components DSL according to CAMEL-13807. Few notes:

  1. To use it to create a component, it can be used like this ComponentsBuilderFactory.kafka().setBrokers("{{host:port}}").build()
  2. Since I needed to maintain the POM file and ComponentsBuilderFactory interface, I used a metadata json file to keep track on the generated DSL classes and update whatever needed.
  3. To set the properties, I used component property configurer since it generates what is needed and I made use of that.
  4. Since I used the metadata file approach, the build speed wasn't impacted much which is good.
  5. If the placeholder needs to be resolved, the component needs to be built with the context being passed as parameter:
    ComponentsBuilderFactory.kafka().setBrokers("{{host:port}}").build(camelContext)
  6. There are a lot of shared functionality between endpoinsDslMojo and componentsDslMojo, hence
    I have created an abstract class for both and helpers that can be shared in both. However, I left EndpointDslMojo untouched for now. Will remove some shared core once I am done from this PR.

Topics need to be done:

  • Fix checkstyle.
  • Add unit tests for the maven plugin generators.
  • Add more unit tests for camel-componentDsl.
  • Optimize some classes on the go.

@omarsmak
Copy link
Member Author

@gnodet I have pushed a new commit with the following fixes:

  • Changed to fluent methods without set.
  • Removed PropertyConfigurerSupport and replaced it with generated setters that is lazley initialized. It happens in the build method.
  • Added a an example using camel-kafka.

Please take a look and let me know.

@lgtm-com
Copy link

lgtm-com bot commented Jan 31, 2020

This pull request introduces 1 alert when merging 928a744 into 7ad529d - view on LGTM.com

new alerts:

  • 1 for Uncontrolled data used in path expression

@davsclaus
Copy link
Contributor

There are a lot of changed files here, would it be possible to just post a snippet of how for example you can use this to configure Kafka, eg you mentioned the camel-kafka-example etc.

@omarsmak
Copy link
Member Author

omarsmak commented Feb 1, 2020

Sure, here is an example:

KafkaComponent kafka = ComponentsBuilderFactory.kafka()
                        .brokers("{{kafka.host}}:{{kafka.port}}")
                        .build(camelContext);

Or from the unit tests:

 KafkaComponent kafkaComponent = ComponentsBuilderFactory.kafka()
                .allowManualCommit(true)
                .configuration(kafkaConfiguration)
                .brokers("localhost:9092")
                .build();

Note: In order to resolve the placeholders, the camel context needs to be passed in the build method

@davsclaus
Copy link
Contributor

Ah thanks, yeah that looks lovely.

For the placeholders then maybe we can have a ComponentsBuilderFactory instance you can dependency inject etc

@Inject
ComponentsBuilderFactory cb

Which will have CamelContext out of the box. But lets wait with this kind and get a first cut implemented first. Looks cool. And it has javadoc on the methods like the endpoint dsl ?

@davsclaus
Copy link
Contributor

Also there could be a method that registers the component to CamelContext with an optional name etc

.register(camelContext, "mykafka");

@omarsmak
Copy link
Member Author

omarsmak commented Feb 1, 2020

Yes, it has everything generated, very similar to the endpointdsl.
Sure, I can add one register API besides build API. So we can have the following:

  • Component build()/ Component build(camelContext) -> Build a component and returns back as return type.
  • void register(camelContext, name) -> Build and register component to the camel context

@dmvolod
Copy link
Member

dmvolod commented Feb 2, 2020

@omarsmak , I'm not sure, but looks like you are missing just one thing, there are set of components (a few but important) which contains set of components inside.
For example camel-mail

but ComponentsBuilderFactory set only imap for camel-mail

@omarsmak
Copy link
Member Author

omarsmak commented Feb 3, 2020

Ahh, thanks @dmvolod for the hints, I will take a look what we can do about.

@omarsmak
Copy link
Member Author

omarsmak commented Feb 3, 2020

@davsclaus @dmvolod I have pushed a new commit that fixes the component aliases not being generated and adds a new register API, e.g:

ComponentsBuilderFactory.kafka()
                        .brokers("{{kafka.host}}:{{kafka.port}}")
                        .register(camelContext, "kafka");

@omarsmak
Copy link
Member Author

omarsmak commented Feb 4, 2020

If there is no further comments, I would like to squash and merge this PR today

@davsclaus
Copy link
Contributor

Yeah I think it seems good, would really like to have this, and we can then improve on it.

@omarsmak omarsmak merged commit c0819a3 into apache:master Feb 4, 2020
@omarsmak omarsmak deleted the CAMEL-13807 branch February 4, 2020 10:16
@davsclaus
Copy link
Contributor

The componentdsl cannot compile - error in camel-hdfs. Also camel-jira needs a 3rd part repo (i may add that to the pom.xml).

@davsclaus
Copy link
Contributor

Also it may be that the generated code in the dsl is not sorted a..z either, eg so when others compile they may get a different order and the code is changed. eg make it sort so components that start with a in the top and so on.

@davsclaus
Copy link
Contributor

Okay with latest code and doing a full rebuild it seems the camel-hdfs issue is gone.

@omarsmak
Copy link
Member Author

omarsmak commented Feb 4, 2020

Yeah that sort issue has to be addressed

@davsclaus
Copy link
Contributor

btw I wonder if we have a catch-22 situation. That the componentdsl, requires all components to be pre-build, eg what if you have a empty m2 repo and build from 100% scratch

@omarsmak
Copy link
Member Author

omarsmak commented Feb 4, 2020

A full rebuild? I am curious about it, let me give a test :D

@lgtm-com
Copy link

lgtm-com bot commented Feb 4, 2020

This pull request introduces 1 alert when merging 908bfc1 into 42adfa6 - view on LGTM.com

new alerts:

  • 1 for Uncontrolled data used in path expression

1 similar comment
@lgtm-com
Copy link

lgtm-com bot commented Feb 4, 2020

This pull request introduces 1 alert when merging 908bfc1 into 42adfa6 - view on LGTM.com

new alerts:

  • 1 for Uncontrolled data used in path expression

@oscerd
Copy link
Contributor

oscerd commented Feb 4, 2020

After merging this each time I rebuild (full rebuild) I'm getting these files modified

#	modified:   core/camel-componentdsl/pom.xml
#	modified:   core/camel-componentdsl/src/main/java/org/apache/camel/builder/component/ComponentsBuilderFactory.java
#	modified:   core/camel-componentdsl/src/main/java/org/apache/camel/builder/component/dsl/SparkComponentBuilderFactory.java
#	modified:   core/camel-componentdsl/src/main/java/org/apache/camel/builder/component/dsl/WebsocketComponentBuilderFactory.java
#	modified:   core/camel-componentdsl/src/main/resources/metadata.json
#	modified:   core/camel-endpointdsl/src/main/java/org/apache/camel/builder/endpoint/dsl/SparkEndpointBuilderFactory.java

@omarsmak
Copy link
Member Author

omarsmak commented Feb 4, 2020

Yeah is because of the order. Pushing a fix for it now to have it ordered

@gnodet
Copy link
Contributor

gnodet commented Feb 4, 2020

@oscerd @omarsmak @davsclaus I'm working on a fix to stabilize the generation while merging those changes in my local branch, I should push very soon.

@omarsmak
Copy link
Member Author

omarsmak commented Feb 4, 2020

Hey @gnodet I pushed a PR #3541 before saw your message to fix it. If you are already pushed it, please feel free to push it and I can close that PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants