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] add sqlserver connector #2646

Merged
merged 17 commits into from
Sep 30, 2022

Conversation

liugddx
Copy link
Member

@liugddx liugddx commented Sep 5, 2022

Purpose of this pull request

support sqlserver connector

Check list

@githublaohu
Copy link

是否应该是jdbc-connent

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.

hi, could you add the doc?

Copy link
Contributor

@ic4y ic4y left a comment

Choose a reason for hiding this comment

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

Sort out all duplicate files. The test needs to check whether the data in the source table and the sink table are consistent

@liugddx liugddx marked this pull request as draft September 8, 2022 15:54
@liugddx liugddx marked this pull request as ready for review September 9, 2022 15:06
@Hisoka-X Hisoka-X requested a review from ic4y September 17, 2022 11:27
@liugddx
Copy link
Member Author

liugddx commented Sep 18, 2022

PTAL @ic4y @EricJoy2048 @CalvinKirs thanks.

@liugddx
Copy link
Member Author

liugddx commented Sep 18, 2022

PTAL @ic4y @EricJoy2048 @CalvinKirs thanks.

e2e test pass
image

ic4y
ic4y previously approved these changes Sep 20, 2022
@ic4y
Copy link
Contributor

ic4y commented Sep 20, 2022

LGTM @Hisoka-X

Co-authored-by: Hisoka <fanjiaeminem@qq.com>
Hisoka-X
Hisoka-X previously approved these changes Sep 20, 2022
@liugddx
Copy link
Member Author

liugddx commented Sep 20, 2022

PTAL @CalvinKirs @TyrantLucifer @EricJoy2048 .thanks

Copy link
Member

@TyrantLucifer TyrantLucifer left a comment

Choose a reason for hiding this comment

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

image

It also has some problems of your pr. Please fix CI first.

@liugddx
Copy link
Member Author

liugddx commented Sep 22, 2022

@liugddx
Copy link
Member Author

liugddx commented Sep 24, 2022

PTAL @CalvinKirs @TyrantLucifer @Hisoka-X .thanks

@hailin0
Copy link
Member

hailin0 commented Sep 29, 2022

LGTM

@@ -32,22 +31,19 @@
* The before method will create a Flink cluster, and after method will close the Flink cluster.
* You can use {@link AbstractFlinkContainer#executeJob} to submit a seatunnel config and run a seatunnel job.
*/
@Slf4j
Copy link
Member Author

Choose a reason for hiding this comment

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

I found out that all the integration tests had failed,and the reason is log be inherited.So i fix it provisionally.

@hailin0 @TyrantLucifer @ashulin

Copy link
Member

Choose a reason for hiding this comment

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

I found out that all the integration tests had failed,and the reason is log be inherited.So i fix it provisionally.

@hailin0 @TyrantLucifer @ashulin

@hailin0 PTAL

Copy link
Member

Choose a reason for hiding this comment

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

I found out that all the integration tests had failed,and the reason is log be inherited.So i fix it provisionally.
@hailin0 @TyrantLucifer @ashulin

@hailin0 PTAL

#2946 has resolved this problem, these files should be reverted.

Copy link
Member Author

Choose a reason for hiding this comment

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

I found out that all the integration tests had failed,and the reason is log be inherited.So i fix it provisionally.
@hailin0 @TyrantLucifer @ashulin

@hailin0 PTAL

#2946 has resolved this problem, these files should be reverted.

yes. I've revert.

@liugddx
Copy link
Member Author

liugddx commented Sep 29, 2022

please rerun CI thanks @TyrantLucifer

@TyrantLucifer
Copy link
Member

please rerun CI thanks @TyrantLucifer

Wait until the running workflow is finished before restarting. Please be patient.

# Conflicts:
#	seatunnel-e2e/seatunnel-connector-v2-e2e/connector-jdbc-e2e/src/test/java/org/apache/seatunnel/connectors/seatunnel/jdbc/JdbcDmdbIT.java
#	seatunnel-e2e/seatunnel-spark-connector-v2-e2e/connector-jdbc-spark-e2e/src/test/java/org/apache/seatunnel/e2e/spark/v2/jdbc/JdbcDmdbIT.java
@liugddx
Copy link
Member Author

liugddx commented Sep 29, 2022

@TyrantLucifer @Hisoka-X PTAL

@@ -32,22 +31,19 @@
* The before method will create a Flink cluster, and after method will close the Flink cluster.
* You can use {@link AbstractFlinkContainer#executeJob} to submit a seatunnel config and run a seatunnel job.
*/
@Slf4j
Copy link
Member

Choose a reason for hiding this comment

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

Please revert this file because pr #2946 has resolved problems of e2e test.

import org.junit.jupiter.api.AfterAll;
import org.junit.jupiter.api.BeforeAll;
import org.junit.jupiter.api.TestInstance;
import org.testcontainers.containers.Container;

import java.io.IOException;

@Slf4j
Copy link
Member

Choose a reason for hiding this comment

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

Please revert this file because pr #2946 has resolved problems of e2e test.

<dependency>
<groupId>org.testcontainers</groupId>
<artifactId>mssqlserver</artifactId>
<version>1.17.3</version>
Copy link
Member

Choose a reason for hiding this comment

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

Using ${testcontainer.version} instead of it.

<dependency>
<groupId>org.testcontainers</groupId>
<artifactId>mssqlserver</artifactId>
<version>1.17.3</version>
Copy link
Member

Choose a reason for hiding this comment

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

Use ${testcontainer.version} instead of it.

Copy link
Member

@TyrantLucifer TyrantLucifer left a comment

Choose a reason for hiding this comment

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

Let's wait CI. Thank you for your contribution.

@liugddx
Copy link
Member Author

liugddx commented Sep 30, 2022

rerun CI? I don't think this mistake affects @TyrantLucifer

@TyrantLucifer
Copy link
Member

rerun CI? I don't think this mistake affects @TyrantLucifer

CI had been optimized, so some steps will be enabled, before your pr is merged CI must run successfully. It's a community norm.

image

There are some problems with the dependency checking step, pls check again.

@Hisoka-X
Copy link
Member

rerun CI? I don't think this mistake affects @TyrantLucifer

CI had been optimized, so some steps will be enabled, before your pr is merged CI must run successfully. It's a community norm.

image

There are some problems with the dependency checking step, pls check again.

At now the dependency have some problem because st-engine bring some dependency but not update LICENSE file, we will update it later.

1 similar comment
@Hisoka-X
Copy link
Member

rerun CI? I don't think this mistake affects @TyrantLucifer

CI had been optimized, so some steps will be enabled, before your pr is merged CI must run successfully. It's a community norm.

image

There are some problems with the dependency checking step, pls check again.

At now the dependency have some problem because st-engine bring some dependency but not update LICENSE file, we will update it later.

@TyrantLucifer
Copy link
Member

rerun CI? I don't think this mistake affects @TyrantLucifer

CI had been optimized, so some steps will be enabled, before your pr is merged CI must run successfully. It's a community norm.
image
There are some problems with the dependency checking step, pls check again.

At now the dependency have some problem because st-engine bring some dependency but not update LICENSE file, we will update it later.

Good, let's waiting other steps of CI. I had restarted the failed steps

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

9 participants