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.

Sign up for GitHub

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

Jump to bottom

[feat][starrocks] add starrocks sync/sql connector #1237

Merged
merged 3 commits into from
Sep 9, 2022
Merged

[feat][starrocks] add starrocks sync/sql connector #1237

merged 3 commits into from
Sep 9, 2022

Conversation

Paddy0523
Copy link
Contributor

Purpose of this pull request

  1. sync : add starrocks source/sink connector
  2. sql : add source/sink/lookup connector

Which issue you fix

Fixes # (issue).

Checklist:

  • I have executed the 'mvn spotless:apply' command to format my code.
  • I have a meaningful commit message (including the issue id, the template of commit message is '[label-type-#issue-id][fixed-module] a meaningful commit message.')
  • I have performed a self-review of my own code.
  • I have commented my code, particularly in hard-to-understand areas.
  • I have made corresponding changes to the documentation.
  • I have added tests that prove my fix is effective or that my feature works.
  • New and existing unit tests pass locally with my changes.
  • I have checked my code and corrected any misspellings.
  • My commit is only one. (If there are multiple commits, you can use 'git squash' to compress multiple commits into one.)

@Paddy0523 Paddy0523 added the good first issue Good for newcomers label Sep 8, 2022
@libailin
Copy link
Contributor

libailin commented Sep 9, 2022

LGTM

FlechazoW
FlechazoW reviewed Sep 9, 2022
View reviewed changes
...src/main/java/com/dtstack/chunjun/connector/starrocks/source/be/StarRocksSourceBeReader.java Outdated
import com.starrocks.shade.org.apache.thrift.protocol.TProtocol;
import com.starrocks.shade.org.apache.thrift.transport.TSocket;
import com.starrocks.shade.org.apache.thrift.transport.TTransportException;
import com.starrocks.thrift.*;
Copy link
Member

Choose a reason for hiding this comment

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

Do not import *.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

FlechazoW
FlechazoW reviewed Sep 9, 2022
View reviewed changes
...c/main/java/com/dtstack/chunjun/connector/starrocks/source/be/StarRocksQueryPlanVisitor.java Outdated

import java.io.IOException;
import java.io.Serializable;
import java.util.*;
Copy link
Member

Choose a reason for hiding this comment

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

Do not import *.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

FlechazoW
FlechazoW reviewed Sep 9, 2022
View reviewed changes
chunjun-connectors/chunjun-connector-starrocks/pom.xml Outdated
<version>5.1.49</version>
</dependency>

<!-- <dependency>-->
Copy link
Member

Choose a reason for hiding this comment

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

Remove useless code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

FlechazoW
FlechazoW reviewed Sep 9, 2022
View reviewed changes
chunjun-connectors/chunjun-connector-starrocks/pom.xml

<dependency>
<groupId>com.alibaba</groupId>
<artifactId>fastjson</artifactId>
Copy link
Member

Choose a reason for hiding this comment

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

Duplicate dependencies are depended on the project, and we have other json-processed dependencies, like Gson, in the root pom.

FlechazoW
FlechazoW reviewed Sep 9, 2022
View reviewed changes
chunjun-connectors/chunjun-connector-starrocks/pom.xml
<dependency>
<groupId>mysql</groupId>
<artifactId>mysql-connector-java</artifactId>
<version>5.1.49</version>
Copy link
Member

Choose a reason for hiding this comment

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

Is this version same as other connector?

@Paddy0523 Paddy0523 force-pushed the feat_master_starrocks branch 2 times, most recently from 6b3b8c7 to 061c85c Compare September 9, 2022 05:44
FlechazoW
FlechazoW reviewed Sep 9, 2022
View reviewed changes
...rocks/src/main/java/com/dtstack/chunjun/connector/starrocks/source/StarRocksInputFormat.java Outdated
}

@Override
protected void closeInternal() {}
Copy link
Member

Choose a reason for hiding this comment

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

Nothing to do here?

.../src/main/java/com/dtstack/chunjun/connector/starrocks/source/StarRocksLruTableFunction.java Outdated Show resolved Hide resolved
.../src/main/java/com/dtstack/chunjun/connector/starrocks/source/StarRocksLruTableFunction.java Show resolved Hide resolved
.../src/main/java/com/dtstack/chunjun/connector/starrocks/source/StarRocksLruTableFunction.java Outdated
}
}
}
beReader.close();
Copy link
Member

Choose a reason for hiding this comment

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

'close()' method should be called in 'finally' block.

...ks/src/main/java/com/dtstack/chunjun/connector/starrocks/source/be/StarRocksArrowReader.java
if (offsetOfBatchForRead < flinkRowsCount) {
return true;
}
this.close();
Copy link
Member

Choose a reason for hiding this comment

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

Why call the 'close()' method here ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed：close in finally

...c/main/java/com/dtstack/chunjun/connector/starrocks/source/be/StarRocksQueryPlanVisitor.java Outdated
.getPartitions()
.forEach(
(tabletId, tablet) -> {
int tabletCount = Integer.MAX_VALUE;
Copy link
Member

Choose a reason for hiding this comment

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

I think this code block should be called in a method.

@FlechazoW
Copy link
Member

Review done.

@FlechazoW FlechazoW merged commit f5aa7d3 into DTStack:master Sep 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@FlechazoW FlechazoW FlechazoW left review comments

@libailin libailin libailin approved these changes

Assignees
No one assigned
Labels
good first issue Good for newcomers
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Paddy0523 @libailin @FlechazoW