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-14096] Adding Kudu component. #3272

Closed
wants to merge 1 commit into from

Conversation

Delawen
Copy link
Contributor

@Delawen Delawen commented Oct 22, 2019

Copy link
Contributor

@oscerd oscerd left a comment

Choose a reason for hiding this comment

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

Please add this to master too.

@Delawen
Copy link
Contributor Author

Delawen commented Oct 22, 2019

@oscerd master is 3.x, right? I was going to work on some 3 branch once this is approved.

@oscerd
Copy link
Contributor

oscerd commented Oct 22, 2019

yes

Copy link
Contributor

@oscerd oscerd left a comment

Choose a reason for hiding this comment

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

The component is missing in parent/pom, camel-bom and apache-camel/ common-descriptor. Also It is missing the Karaf feature and integration test for Spring Boot and Karaf support. There is a bit more work needed. Thanks for the contritbuion

<schemeName>kudu</schemeName>
<componentName>Apache Kudu</componentName>
<componentPackage>org.apache.camel.component.kudu</componentPackage>
<kuduVersion>1.10.0</kuduVersion>
Copy link
Contributor

Choose a reason for hiding this comment

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

please add this properties under parent/pom

<componentName>Apache Kudu</componentName>
<componentPackage>org.apache.camel.component.kudu</componentPackage>
<kuduVersion>1.10.0</kuduVersion>
<slf4jVersion>1.7.25</slf4jVersion>
Copy link
Contributor

Choose a reason for hiding this comment

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

This can be removed I guess

* <a href="https://kudu.apache.org/">Apache Kudu</a>, a free and open source
* column-oriented data store of the Apache Hadoop ecosystem.
*/
@UriEndpoint(firstVersion = "2.23.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

First version shoud be 2.25.0

/**
* Set the client to connect to a kudu resource
*
* @param kuduClient
Copy link
Contributor

Choose a reason for hiding this comment

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

all the @param annotations can be removed

String key = (String) entry.getKey();
Object value = entry.getValue();
switch (value.getClass().toString()) {
case "class java.lang.String":
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't a better way to express this, it's really ugly :-(

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 thought the same when I saw it :) Will look for something better. It looks like a list of if-elses with instanceof is an antipattern.

@@ -0,0 +1,104 @@
{
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not useful for camel and it can be removed. Please have a look at the other component structure.

@Delawen
Copy link
Contributor Author

Delawen commented Oct 22, 2019

Thank you!

@oscerd
Copy link
Contributor

oscerd commented Oct 25, 2019

By the way I would first add this to master. I'm not sure this is really needed on 2.x, since we're moving to camel 3

@Delawen
Copy link
Contributor Author

Delawen commented Oct 25, 2019

Ok, I can move and work on master. Will take some more time but can do.

Still working on the tests, reading and making sure I don't miss anything.

Backported from Syndesis and adapting to Camel standards and
requirements.

Co-authored-by: Luis Garcia Acosta <lgarciaac@gmail.com>
title = "Apache Kudu", syntax = "kudu:type",
consumerClass = KuduConsumer.class,
label = "database,iot")
public class KuduEndpoint extends DefaultEndpoint {
Copy link
Member

@dmvolod dmvolod Oct 28, 2019

Choose a reason for hiding this comment

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

@Delawen may be rethink about enpoint syntax from separate URI parameters (host and port) to the syntax like in thrift or similar components, i.e. thrift:host:port/service.
for this component it would like kudu:host:port/type that much more cloud ready for services and routes.
Also KuduClientBuilder is using this syntax.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense. Thanks!

@Delawen
Copy link
Contributor Author

Delawen commented Oct 28, 2019

I made it work in Camel 2.25 (this branch) but I am going to work on a PR for 3.x.

<dependency>
<groupId>org.apache.kudu</groupId>
<artifactId>kudu-client</artifactId>
<version>${kuduVersion}</version>
Copy link
Member

Choose a reason for hiding this comment

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

It would be nice to change to the common format for all components, i.e. kudu-version

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's already done. I'm going to close this PR and focus on the 3.x branch :)

KuduClient connection = endpoint.getKuduClient();
LOG.debug("Creating table {}", tableName);

Schema schema = (Schema) exchange.getIn().getHeader("Schema");
Copy link
Member

Choose a reason for hiding this comment

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

It would be nice to have headers name in Camel style like CamelKuduSchema and store them names in separate constant class/interface.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks!

@Delawen
Copy link
Contributor Author

Delawen commented Oct 29, 2019

Moved effort to #3290 @oscerd @dmvolod

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