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

[FLINK-35638] Refactor OceanBase test cases and remove dependency on host network #3439

Merged
merged 1 commit into from
Aug 18, 2024

Conversation

whhe
Copy link
Member

@whhe whhe commented Jun 27, 2024

No description provided.

@whhe whhe force-pushed the ob-test-network branch 3 times, most recently from 163b0e0 to 52006b1 Compare June 27, 2024 14:29
@whhe whhe force-pushed the ob-test-network branch 4 times, most recently from 87a9cc9 to 06afe10 Compare July 2, 2024 06:47
@github-actions github-actions bot added the docs Improvements or additions to documentation label Jul 2, 2024
@whhe whhe force-pushed the ob-test-network branch 3 times, most recently from 9be64dc to b1edf22 Compare July 2, 2024 14:21
@whhe
Copy link
Member Author

whhe commented Jul 3, 2024

@GOODBOY008 @ruanhang1993 PTAL

@leonardBang
Copy link
Contributor

@GOODBOY008 would you like to take a look at this PR when you have time?

@whhe whhe force-pushed the ob-test-network branch 2 times, most recently from 11e23b5 to 64ea6f1 Compare August 1, 2024 06:14
@whhe
Copy link
Member Author

whhe commented Aug 1, 2024

Doc typo fixed, @GOODBOY008 PTAL.

@whhe whhe requested a review from GOODBOY008 August 7, 2024 07:01

public Connection getJdbcConnection() throws SQLException {
return DriverManager.getConnection(
container.getJdbcUrl(databaseName), getUsername(), getPassword());
Copy link
Member

Choose a reason for hiding this comment

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

@whhe It's wrong to get connection here, should be the implement like : org.apache.flink.cdc.connectors.oceanbase.OceanBaseTestBase#getJdbcConnection.

Copy link
Member Author

Choose a reason for hiding this comment

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

It is copied from UniqueDatabase in the mysql-cdc test module, and the database name it connects to is different from OceanBaseTestBase#getJdbcConnection. I think we can keep it.

Copy link
Member

Choose a reason for hiding this comment

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

If ob in oracle model and e2e test invork UniqueDatabase.getJdbcConnection, what will happened?

Copy link
Member Author

Choose a reason for hiding this comment

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

UniqueDatabase is instanced by a container object, so it can't be used for oracle mode now.

Copy link
Member

Choose a reason for hiding this comment

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

So why not unify invoke org.apache.flink.cdc.connectors.oceanbase.OceanBaseTestBase#getJdbcConnection ?

Copy link
Member Author

@whhe whhe Aug 15, 2024

Choose a reason for hiding this comment

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

I didn't get you. The e2e test case is Parameterized, so I added the UniqueDatabase to create tables for different parameters. It's not a sub class of OceanBaseTestBase, if I want to connect to the unique database, it's straightforward to have a getJdbcConnection method in UniqueDatabase.

Copy link
Member

@GOODBOY008 GOODBOY008 left a comment

Choose a reason for hiding this comment

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

LGTM

@GOODBOY008 GOODBOY008 merged commit cbb33bb into apache:master Aug 18, 2024
28 checks passed
qiaozongmi pushed a commit to qiaozongmi/flink-cdc that referenced this pull request Sep 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Improvements or additions to documentation e2e-tests oceanbase-cdc-connector
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants