-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
issues#1946 add [Druid] Source & Sink #2544
Conversation
fix:issues#1946 add [Druid] Source & Sink |
fix:issues#1946 add [Druid] Source & Sink |
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.
- Please add the license header.
- Maybe you can add druid dialect in jdbc connector.
@@ -0,0 +1,58 @@ | |||
<?xml version="1.0" encoding="UTF-8"?> | |||
<project xmlns="http://maven.apache.org/POM/4.0.0" |
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.
add license header
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.
done
@@ -0,0 +1,201 @@ | |||
package org.apache.seatunnel.connectors.seatunnel.druid.client; |
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.
same above
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.
done
@ic4y PTAL, thx. |
<dependency> | ||
<groupId>org.apache.druid</groupId> | ||
<artifactId>druid-indexing-service</artifactId> | ||
<scope>compile</scope> |
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.
We already have the class JsonUtil, please use this.
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.
In JsonUtil OBJECT_MAPPER configure state cannot be shared.need new another ObjectMapper
<dependency> | ||
<groupId>org.apache.seatunnel</groupId> | ||
<artifactId>connector-common</artifactId> | ||
<version>${project.version}</version> | ||
<scope>provided</scope> | ||
</dependency> | ||
<dependency> | ||
<groupId>org.apache.httpcomponents</groupId> | ||
<artifactId>httpclient</artifactId> | ||
<version>4.5.13</version> | ||
<scope>compile</scope> | ||
</dependency> | ||
<dependency> | ||
<groupId>org.apache.calcite.avatica</groupId> | ||
<artifactId>avatica-core</artifactId> | ||
<version>1.15.0</version> | ||
<scope>compile</scope> | ||
</dependency> | ||
<dependency> | ||
<groupId>org.apache.druid</groupId> | ||
<artifactId>druid-core</artifactId> | ||
<version>RELEASE</version> | ||
<scope>compile</scope> |
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.
The dependency version should manage by root pom
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.
done
} catch (Exception e) { | ||
LOGGER.warn("get row type info exception", e); |
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.
Should throw exception?
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.
done
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.
Can you add connector-v2 e2e-testcase & docs
reference:#2499
/** | ||
* guanbo | ||
*/ |
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.
delete meaningless comments
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.
done
/** | ||
* guanbo | ||
*/ |
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.
same above
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.
done
/** | ||
* guanbo | ||
*/ |
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.
Check all guanbo
places, delete it.
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.
done
…ions columns type
…mat columns type
Hi, please resolve conflicts files |
done |
…k doc and DEFAULT_TIMESTAMP_MISSING_VALUE assign initial value
done add source & sink doc |
@@ -26,7 +26,7 @@ env { | |||
spark.executor.instances = 2 | |||
spark.executor.cores = 1 | |||
spark.executor.memory = "1g" | |||
spark.stream.batchDuration = 5 | |||
spark.streaming.batchDuration = 5 |
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.
Please check this #2575
@@ -0,0 +1,101 @@ | |||
# Druid |
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.
Please update doc to match new format, reference: #2625
@@ -35,12 +35,6 @@ Otherwise, your code could not start in JetBrains IntelliJ IDEA correctly. | |||
./mvnw install -Dmaven.test.skip | |||
``` | |||
|
|||
### Building SeaTunnel from source |
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.
Why remove this?
@@ -34,7 +34,7 @@ ln -s seatunnel-<version> seatunnel | |||
```bash | |||
env { | |||
# seatunnel defined streaming batch duration in seconds | |||
spark.stream.batchDuration = 5 |
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.
same as above
@@ -204,6 +204,11 @@ | |||
<p3c-pmd.version>1.3.0</p3c-pmd.version> | |||
<maven-scm-provider-jgit.version>1.9.5</maven-scm-provider-jgit.version> | |||
<testcontainer.version>1.16.3</testcontainer.version> | |||
<jackson-datatype-joda.version>2.12.1</jackson-datatype-joda.version> |
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.
Don't add the connector's dependency in root pom, just use it in connector's pom.xml, reference: https://github.com/apache/incubator-seatunnel/pull/2630/files
@@ -48,12 +48,9 @@ class SparkStreamingExecution(sparkEnvironment: SparkEnvironment) | |||
dataset) | |||
} | |||
var ds = dataset | |||
|
|||
if (ds.take(1).length > 0) { |
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.
Can't delete this, check this: #2578
I'm sorry, but I want reject this PR. Because too many code update but no connection with Druid Source/Sink. You can split it into multi PR, each have only one main purpose. Thanks again! |
And you delete lots of code which shouldn't be deleted. |
Any update you can reopen this PR again, or create another PR only with Druid Source/Sink (recommend). |
OK THX! |
done |
Purpose of this pull request
Check list
New License Guide