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-28433][connector/jdbc]Add mariadb jdbc connection validation #20193

Closed
wants to merge 1 commit into from

Conversation

bzhaoopenstack
Copy link
Contributor

@bzhaoopenstack bzhaoopenstack commented Jul 7, 2022

What is the purpose of the change

  1. Flink Connector support the jdbc connection with Mysql series DB, such as MySQL, MariaDB,
    Percona and etc. But we see the doc reference[1] shows the Mariadb Connector/J 3.0 will
    accepts jdbc:mariadb: as the protocal in connection strings by default. So we need to apply
    this change in our mysql jdbc connection vailidation. Mariadb Connector/J 2.X still support
    both "jdbc:mysql:" and "jdbc:mariadb:".
  2. Introduce MariaDB dialect implemented like Mysql.
  3. Add UTs for DataTypes and a testcontainer for MariaDB in
    JdbcExactlyOnceSinkE2eTest.

[1] https://mariadb.com/kb/en/about-mariadb-connector-j/#jdbcmysql-scheme-compatibility

Brief change log

  • Making Mysql jdbc connector validate the MariaDB type connection protocol for fit users to use Mysql jdbc connector to access MariaDB instances.
  • Introduce MariaDB jdbc dialect into Flink, which is very similar with Mysql.
  • Introduce UTs and E2E test for them.

Verifying this change

Exec mvn clean package -Dfast -Pskip-webui-build -pl flink-connectors/flink-connector-jdbc
Or mvn clean package -Dfast -Pskip-webui-build -pl flink-connectors/flink-connector-jdbc -Dtest="org.apache.flink.connector.jdbc.xa.JdbcExactlyOnceSinkE2eTest" for run all tests or a single test.
Notes that all types didn't cover, and will be on going for support.

Does this pull request potentially affect one of the following parts:

  • Dependencies (does it add or upgrade a dependency): (no)
  • The public API, i.e., is any changed class annotated with @Public(Evolving): (no)
  • The serializers: (no)
  • The runtime per-record code paths (performance sensitive): (no)
  • Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Kubernetes/Yarn, ZooKeeper: (no)
  • The S3 file system connector: (no)

Documentation

  • Does this pull request introduce a new feature? (no)
  • If yes, how is the feature documented? (docs)

@flinkbot
Copy link
Collaborator

flinkbot commented Jul 7, 2022

CI report:

Bot commands The @flinkbot bot supports the following commands:
  • @flinkbot run azure re-run the last Azure build

@Peng-Lei
Copy link

Peng-Lei commented Jul 7, 2022

+1, After the change. We can use mariadb driver to connect the mysql through by setting the jdbcOptions of driver parameter without change the url prefix.

Copy link
Contributor

@MartijnVisser MartijnVisser left a comment

Choose a reason for hiding this comment

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

@bzhaoopenstack Thanks for the PR, but in order to add support for MariaDB I would also like to see a test and documentation in place.

@MartijnVisser MartijnVisser self-assigned this Jul 7, 2022
@bzhaoopenstack
Copy link
Contributor Author

@bzhaoopenstack Thanks for the PR, but in order to add support for MariaDB I would also like to see a test and documentation in place.

Thanks very much for reply and suggest. I will add the test and documentation. Once I'm finish, I think I need your kind review and advice. Thank you. ;-)

@bzhaoopenstack bzhaoopenstack changed the title [FLINK-28433][connector/jdbc]Add mariadb jdbc connection validation [WIP][FLINK-28433][connector/jdbc]Add mariadb jdbc connection validation Jul 8, 2022
@bzhaoopenstack
Copy link
Contributor Author

@bzhaoopenstack Thanks for the PR, but in order to add support for MariaDB I would also like to see a test and documentation in place.

Hi @MartijnVisser, I have a question here, you said you want a test for this PR, does it mean we should add a specific UT which only verify the jdbc conncetion strings? Because I didn't find any existing UT in Flink.
And notes: We only use Mysql connector to connect MariaDB, as it is a Mysql-series database.

@MartijnVisser
Copy link
Contributor

I would expect that we add MariaDB to the JdbcExactlyOnceSinkE2eTest with the correct test container and have a MariaDB dialect implemented (even if that would be just a copy of MySQL)

@bzhaoopenstack
Copy link
Contributor Author

I would expect that we add MariaDB to the JdbcExactlyOnceSinkE2eTest with the correct test container and have a MariaDB dialect implemented (even if that would be just a copy of MySQL)

Thanks for explain more, this is a good direction/suggestion for me.

@bzhaoopenstack
Copy link
Contributor Author

Hi @MartijnVisser , sorry for late.
And this is my first PR in flink, so I wish you could leave some kind directions and suggestions. Thank you very much. ;-). And hope this doesn't disturb you so much.

Now, I will request an new review from your side, once we both think this PR is ready, I will remove the title [WIP]. ;-)

@@ -27,7 +27,7 @@
public class MySqlDialectFactory implements JdbcDialectFactory {
@Override
public boolean acceptsURL(String url) {
return url.startsWith("jdbc:mysql:");
return (url.startsWith("jdbc:mysql:") || url.startsWith("jdbc:mariadb:"));

Choose a reason for hiding this comment

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

I think no need change here after we add MariaDBDialectFactory

@MartijnVisser
Copy link
Contributor

@afedulov Do you want to have a look at this one?

@bzhaoopenstack
Copy link
Contributor Author

Hi all. @MartijnVisser @hadronzoo any updates on this one? Thanks

@Peng-Lei
Copy link

@bzhaoopenstack Are you ready?If yes, please change the title without "WIP".

@bzhaoopenstack
Copy link
Contributor Author

@bzhaoopenstack Are you ready?If yes, please change the title without "WIP".

Yeah, I'm ready for the kind suggest from @MartijnVisser and @hadronzoo . ;-)

@MartijnVisser
Copy link
Contributor

@bzhaoopenstack I won't be able to review this at the moment, but keep in mind that the CI is still failing for this PR. It makes sense to make sure that the CI works before the review actually starts.

@bzhaoopenstack
Copy link
Contributor Author

@bzhaoopenstack I won't be able to review this at the moment, but keep in mind that the CI is still failing for this PR. It makes sense to make sure that the CI works before the review actually starts.

OK, thank you Martijn. I will resolve it and make it ready for review

@bzhaoopenstack
Copy link
Contributor Author

Sorry for late update. ;-) . Let's wait the azure test result. I got pass on my local env.

1. Flink Connector support the jdbc connection with Mysql series DB, such as MySQL, MariaDB,
Percona and etc. But we see the doc reference[1] shows the Mariadb Connector/J 3.0 will
accepts jdbc:mariadb: as the protocal in connection strings by default. So we need to apply
this change in our mysql jdbc connection vailidation. Mariadb Connector/J 2.X still support
both "jdbc:mysql:" and "jdbc:mariadb:".
2. Introduce MariaDB dialect implemented like Mysql.
3. Add UTs for DataTypes and a testcontainer for MariaDB in
   JdbcExactlyOnceSinkE2eTest.

[1] https://mariadb.com/kb/en/about-mariadb-connector-j/#jdbcmysql-scheme-compatibility
@bzhaoopenstack
Copy link
Contributor Author

@flinkbot run azure

@bzhaoopenstack bzhaoopenstack changed the title [WIP][FLINK-28433][connector/jdbc]Add mariadb jdbc connection validation [FLINK-28433][connector/jdbc]Add mariadb jdbc connection validation Aug 24, 2022
@MartijnVisser
Copy link
Contributor

We've now moved the code from the JDBC connector to its own dedicated repository; please re-route this PR to https://github.com/apache/flink-connector-jdbc

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants