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

[Feature][Connector-V2]Support datahub sink #2558

Merged
merged 32 commits into from
Aug 31, 2022
Merged

Conversation

chessplay
Copy link
Contributor

@chessplay chessplay commented Aug 29, 2022

**Support datahub sink #1946

Purpose of this pull request

Check list

@chessplay
Copy link
Contributor Author

@CalvinKirs please help me review the datahub connector code, the previous pr #2529 has been closed, I have optimized the code as your advice. Example and unit case of this connector is not convenient to provide,because it need to personal datahub account ,include a accessKey and accessSecret.

@chessplay chessplay changed the title [Feature][File connector] Support datahub sink [Feature]Support datahub sink Aug 29, 2022
@chessplay chessplay changed the title [Feature]Support datahub sink [Feature][Connector-V2]Support datahub sink Aug 29, 2022
Copy link
Member

@EricJoy2048 EricJoy2048 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 e2e test for this connector.
Need add this connector to seatunnel-connector-v2-dist/pom.xml

| retryTimes | int | yes | - |

### url [string]
i
Copy link
Member

Choose a reason for hiding this comment

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

What's the i mean?

int i = result.getFailedRecordCount();
if (i > 0) {
LOG.info("begin to retry for putting failed record");
if (retry(result.getFailedRecords(), retryTimes, project, topic)) {
Copy link
Member

Choose a reason for hiding this comment

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

I suggest use RetryUtils to do retry.

Comment on lines 37 to 41
<dependency>
<groupId>com.aliyun.datahub</groupId>
<artifactId>aliyun-sdk-datahub</artifactId>
<version>2.19.0-public</version>
</dependency>
Copy link
Member

Choose a reason for hiding this comment

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

hi, Our versions are managed uniformly in the root pom (dependency.managent)

@@ -121,4 +121,5 @@ seatunnel.sink.IoTDB = connector-iotdb
seatunnel.sink.Neo4j = connector-neo4j
seatunnel.sink.FtpFile = connector-file-ftp
seatunnel.sink.Socket = connector-socket
seatunnel.sink.datahub = connector-datahub
Copy link
Member

Choose a reason for hiding this comment

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

seatunnel.sink.datahubrename to seatunnel.sink.DataHub it's better,We'd better be consistent with pluginname

*/
public class DataHubWriter extends AbstractSinkWriter<SeaTunnelRow, Void> {

private static final Logger LOG = LoggerFactory.getLogger(DataHubWriter.class);
Copy link
Member

Choose a reason for hiding this comment

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

We can use lombok's @slf4j annotation instead of manually declaring

}

# If you would like to get more information about how to configure seatunnel and see full list of sink plugins,
# please go to https://seatunnel.apache.org/docs/category/sink-v2
Copy link
Member

Choose a reason for hiding this comment

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

This url is not right.

https://seatunnel.apache.org/docs/connector-v2/sink may more suitable

@@ -1,4 +1,4 @@
COMMON DEVELOPMENT AND DISTRIBUTION LICENSE (CDDL) Version 1.0
COMMON DEVELOPMENT AND DISTtRIBUTION LICENSE (CDDL) Version 1.0
Copy link
Member

Choose a reason for hiding this comment

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

please check this.

@CalvinKirs
Copy link
Member

@CalvinKirs please help me review the datahub connector code, the previous pr #2529 has been closed, I have optimized the code as your advice. Example and unit case of this connector is not convenient to provide,because it need to personal datahub account ,include a accessKey and accessSecret.

Hi, You are right, can you disable your e2e and then delete your key or replace it with XXX. I'm concerned about the security of your account key.

@chessplay
Copy link
Contributor Author

local e2e test
image

EricJoy2048
EricJoy2048 previously approved these changes Aug 30, 2022
Copy link
Member

@EricJoy2048 EricJoy2048 left a comment

Choose a reason for hiding this comment

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

LGTM

Comment on lines 55 to 57
endpoint="http://dh-cn-hangzhou.aliyuncs.com"
accessId="LTAIMHD6kepeaI53"
accessKey="SoEU2dIeVGBIfM3LSB8G7QSrwGZLlO"
Copy link
Member

Choose a reason for hiding this comment

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

hi,for security reasons, you need to replace the key information with XXXhi,


import java.io.IOException;

@Disabled
Copy link
Member

Choose a reason for hiding this comment

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

Hi, can you add a comment to explain why?

Copy link
Member

@CalvinKirs CalvinKirs left a comment

Choose a reason for hiding this comment

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

LGTM, thx

@CalvinKirs CalvinKirs merged commit 43600a7 into apache:dev Aug 31, 2022
MRYOG pushed a commit to MRYOG/incubator-seatunnel that referenced this pull request Sep 8, 2022
Co-authored-by: zhoulw11 <zhoulw11@chinatelecom.cn>
MRYOG pushed a commit to MRYOG/incubator-seatunnel that referenced this pull request Sep 15, 2022
Co-authored-by: zhoulw11 <zhoulw11@chinatelecom.cn>
MRYOG pushed a commit to MRYOG/incubator-seatunnel that referenced this pull request Sep 16, 2022
Co-authored-by: zhoulw11 <zhoulw11@chinatelecom.cn>
TyrantLucifer pushed a commit to TyrantLucifer/incubator-seatunnel that referenced this pull request Sep 18, 2022
Co-authored-by: zhoulw11 <zhoulw11@chinatelecom.cn>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants