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

[Connector-V2] Add Kudu source and sink connector #2254

Merged
merged 23 commits into from
Aug 5, 2022

Conversation

2013650523
Copy link
Contributor

@2013650523 2013650523 commented Jul 23, 2022

Purpose of this pull request

Check list

@CalvinKirs CalvinKirs changed the title [api draft] [connector] Add Kudu source and sink connector [Connector] Add Kudu source and sink connector Jul 24, 2022
@CalvinKirs CalvinKirs added the connectors-v1 SeaTunnel connectors, include sink, source label Jul 24, 2022
@@ -102,6 +102,8 @@ seatunnel.sink.Clickhouse = connector-clickhouse
seatunnel.sink.ClickhouseFile = connector-clickhouse
seatunnel.source.Jdbc = connector-jdbc
seatunnel.sink.Jdbc = connector-jdbc
seatunnel.source.Kudu = connector-Kudu
Copy link
Member

Choose a reason for hiding this comment

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

should be seatunnel.source.Kudu = connector-kudu

Comment on lines 1 to 3
<?xml version="1.0" encoding="UTF-8"?>
<project xmlns="http://maven.apache.org/POM/4.0.0"
xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
Copy link
Member

Choose a reason for hiding this comment

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

hi, you forget the license header.

Comment on lines 108 to 110
} catch (Exception e) {
logger .warn("get row type info exception", e);
throw new PrepareFailException("kudu", PluginType.SOURCE, e.toString());
Copy link
Member

Choose a reason for hiding this comment

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

It's better to add an exception Message.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

throw new PrepareFailException("kudu", PluginType.SOURCE, e.toString()); I don't quite understand,There's an exception message in this one

@EricJoy2048
Copy link
Member

Can you change this pr title to [Connector-V2] ?

@2013650523 2013650523 changed the title [Connector] Add Kudu source and sink connector [Connector-V2] Add Kudu source and sink connector Jul 25, 2022
@2013650523
Copy link
Contributor Author

Can you change this pr title to [Connector-V2] ?

ok,tks

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.

Some suggestions


public KuduSinkConfig(@NonNull Config pluginConfig) {

this.saveMode = StringUtils.isBlank(pluginConfig.getString(KUDU_SAVE_MODE)) ? SaveMode.APPEND : SaveMode.fromStr(pluginConfig.getString(KUDU_SAVE_MODE));
Copy link
Member

Choose a reason for hiding this comment

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

There are some problem in this line

pluginConfig.getString("xxxx") will throw NullPointException when xxxx not in config file. So we need judge if xxxx is config file by pluginConfig.hasPath("xxxx").

schema = kuduClient.openTable(tableName).getSchema();
keyColumn = schema.getPrimaryKeyColumns().get(0).getName();
columns =schema.getColumns();
} catch (KuduException e) {
Copy link
Member

Choose a reason for hiding this comment

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

If the KuduException be catch, this method will return null. But I found you didn't handle the null value when you use the return value of this method.

import java.util.List;
import java.util.Map;

public class KuduSinkAggregatedCommitter implements SinkAggregatedCommitter<KuduCommitInfo, KuduAggregatedCommitInfo> {
Copy link
Member

Choose a reason for hiding this comment

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

If the connector don't need store state and 2PC , You can only extends AbstractSimpleSink and that will be simply.

@CalvinKirs CalvinKirs added connectors-v2 and removed connectors-v1 SeaTunnel connectors, include sink, source labels Jul 27, 2022
@Hisoka-X
Copy link
Member

Hisoka-X commented Aug 1, 2022

Please resolve ci error. Thanks

@Hisoka-X Hisoka-X requested review from CalvinKirs, ashulin and EricJoy2048 and removed request for ashulin and EricJoy2048 August 2, 2022 10:11
@2013650523
Copy link
Contributor Author

Hi,@CalvinKirs I have finished the modification and provided the document, please help to review

EricJoy2048
EricJoy2048 previously approved these changes Aug 3, 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.

Comment on lines 25 to 45
public static String getMessage(Throwable e) {
StringWriter sw = null;
PrintWriter pw = null;
try {
sw = new StringWriter();
pw = new PrintWriter(sw);
// Output the error stack information to the printWriter
e.printStackTrace(pw);
pw.flush();
sw.flush();
} finally {
if (sw != null) {
try {
sw.close();
} catch (IOException e1) {
e1.printStackTrace();
}
}
if (pw != null) {
pw.close();
}
Copy link
Member

Choose a reason for hiding this comment

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

used try-with-resources to simplify this

@Hisoka-X Hisoka-X merged commit 0483cbc into apache:dev Aug 5, 2022
TyrantLucifer pushed a commit to TyrantLucifer/incubator-seatunnel that referenced this pull request Sep 18, 2022
* 0

* Update pom.xml

add kudu dependency

* Update plugin-mapping.properties

add kudu config

* Update pom.xml

add kudu

* add email sink connector

* Delete seatunnel-connectors-v2/connector-email directory

* Update plugin-mapping.properties

* Update plugin-mapping.properties

* Update pom.xml

* Create pom.xml

* [Connector-V2] Add Kudu source and sink connector

* [Connector-V2] Add Kudu source and sink connector

* [Connector-V2] update

* [Connector-V2] solve ci error

* [Connector-V2] fix problem on code review

* [Connector-V2] add kudu usage document and fix problem on code review

* Update ExceptionUtil.java

* Update pom.xml

* Update Kudu.md

* Update ExceptionUtil.java

Co-authored-by: Hisoka <fanjiaeminem@qq.com>
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.

5 participants