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

[Improve][Connector-v2][Jdbc] Refactor AbstractJdbcCatalog #5096

Merged
merged 2 commits into from
Sep 6, 2023

Conversation

whhe
Copy link
Member

@whhe whhe commented Jul 17, 2023

Purpose of this pull request

Extract the common logic of MySqlCatalog and SqlServerCatalog to AbstractJdbcCatalog.

Check list

@whhe whhe changed the title [Feature][Connector-v2][Jdbc] Refactor AbstractJdbcCatalog [Improve][Connector-v2][Jdbc] Refactor AbstractJdbcCatalog Jul 17, 2023
@TyrantLucifer
Copy link
Member

Please add unit test cases.

@whhe
Copy link
Member Author

whhe commented Jul 18, 2023

Please add unit test cases.

Seems SeaTunnel does not depend on Mockito now, I want to introduce it to mock jdbc connection, is it OK ? And if so what version should I choose?

@liugddx
Copy link
Member

liugddx commented Jul 19, 2023

Mockito

Refer to seatunnel-e2e/connector-jdbc-e2e

@whhe
Copy link
Member Author

whhe commented Jul 19, 2023

Mockito

Refer to seatunnel-e2e/connector-jdbc-e2e

So I should add IT cases in module connector-jdbc-e2e instead of UT cases in the modified module, am I right?

@EricJoy2048
Copy link
Member

Mockito

Refer to seatunnel-e2e/connector-jdbc-e2e

So I should add IT cases in module connector-jdbc-e2e instead of UT cases in the modified module, am I right?

Yes, you can add IT for this pr.

@ic4y
Copy link
Contributor

ic4y commented Aug 5, 2023

@whhe Great commit! Now we just need to resolve the conflicts.

@whhe whhe force-pushed the jdbc-catalog branch 3 times, most recently from ee16086 to 1c0971b Compare August 7, 2023 08:36
@whhe
Copy link
Member Author

whhe commented Aug 7, 2023

I rebased the code and seems it's coverd by the IT cases for now. @ic4y @EricJoy2048 @liugddx

@ic4y
Copy link
Contributor

ic4y commented Aug 8, 2023

@XiaoJiang521 PTAL

@whhe
Copy link
Member Author

whhe commented Aug 8, 2023

The failed case JdbcPhoenixIT seems not related to this pr.

@whhe
Copy link
Member Author

whhe commented Aug 8, 2023

@XiaoJiang521 Comments addressed, what do you think for now.

@XiaoJiang521
Copy link
Contributor

LGTM

@whhe
Copy link
Member Author

whhe commented Aug 9, 2023

@ic4y @EricJoy2048 PTAL

@ic4y
Copy link
Contributor

ic4y commented Aug 9, 2023

At present, no tests cover these modifications, and the UT tests are also not working. We should add a seatunnel-catalog-e2e module under the seatunnel-e2e module and use testcontainers to cover these tests.

@whhe
Copy link
Member Author

whhe commented Aug 9, 2023

At present, no tests cover these modifications, and the UT tests are also not working. We should add a seatunnel-catalog-e2e module under the seatunnel-e2e module and use testcontainers to cover these tests.

Finally got caught with this work again ... will try it later.

@whhe whhe force-pushed the jdbc-catalog branch 3 times, most recently from a586988 to 159999c Compare August 11, 2023 08:07
@EricJoy2048 EricJoy2048 added this to the 2.3.4 milestone Aug 22, 2023
@whhe whhe force-pushed the jdbc-catalog branch 3 times, most recently from 1c3be7f to be653d4 Compare August 28, 2023 08:40
@whhe
Copy link
Member Author

whhe commented Aug 28, 2023

I add testCatalog method in AbstractJdbcIT and enable it for MySql, Oracle and SqlServer in this pr, looks like these IT cases finally passed.

As PG does not use the same base IT class, I want to modify it in another pr, what do you think? @ic4y

@whhe whhe force-pushed the jdbc-catalog branch 2 times, most recently from e5957f6 to 66d95bb Compare August 29, 2023 04:53
@whhe
Copy link
Member Author

whhe commented Aug 29, 2023

Thanks to help from @EricJoy2048, CI jobs are totally green now.

@whhe
Copy link
Member Author

whhe commented Aug 30, 2023

All catalog implementations related to AbstractJdbcCatalog are covered by IT cases now, and UT failure is not related.

PTAL @ic4y @liugddx @EricJoy2048

@liugddx
Copy link
Member

liugddx commented Aug 30, 2023

All catalog implementations related to AbstractJdbcCatalog are covered by IT cases now, and UT failure is not related.

PTAL @ic4y @liugddx @EricJoy2048

Waiting for #5403 to be merged

@whhe
Copy link
Member Author

whhe commented Aug 31, 2023

@liugddx Could you please help to retry the failed ut case? Seems the testJobLevelCheckpointTimeOut is not completely fixed.

@EricJoy2048 EricJoy2048 closed this Sep 1, 2023
@EricJoy2048 EricJoy2048 reopened this Sep 1, 2023
@liugddx
Copy link
Member

liugddx commented Sep 1, 2023

@liugddx Could you please help to retry the failed ut case? Seems the testJobLevelCheckpointTimeOut is not completely fixed.

Yes, I found the problem, it's not very stable. I will fix this problem in other PR #5414

@whhe
Copy link
Member Author

whhe commented Sep 1, 2023

All tests passed, PTAL @ic4y @liugddx @EricJoy2048

@ic4y
Copy link
Contributor

ic4y commented Sep 5, 2023

@XiaoJiang521 is familiar with this area, could you please review it?

@XiaoJiang521
Copy link
Contributor

LGTM @ic4y

@EricJoy2048 EricJoy2048 merged commit dde3104 into apache:dev Sep 6, 2023
Zhouwen-CN pushed a commit to Zhouwen-CN/seatunnel that referenced this pull request Sep 11, 2023
* refactor jdbc connector AbstractJdbcCatalog

* add testCatalog for pg
Zhouwen-CN pushed a commit to Zhouwen-CN/seatunnel that referenced this pull request Sep 11, 2023
* refactor jdbc connector AbstractJdbcCatalog

* add testCatalog for pg
Zhouwen-CN pushed a commit to Zhouwen-CN/seatunnel that referenced this pull request Sep 11, 2023
* refactor jdbc connector AbstractJdbcCatalog

* add testCatalog for pg
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.

6 participants