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

[FLINK-35363] FLIP-449: Reorganization of flink-connector-jdbc #123

Merged
merged 14 commits into from
Jul 29, 2024
Merged

[FLINK-35363] FLIP-449: Reorganization of flink-connector-jdbc #123

merged 14 commits into from
Jul 29, 2024

Conversation

eskabetxe
Copy link
Member

@eskabetxe eskabetxe commented May 29, 2024 •
edited
Loading

@eskabetxe eskabetxe changed the title [FLINK-35363] FLIP-449: Reorganization of flink-connector-jdbc [WIP][FLINK-35363] FLIP-449: Reorganization of flink-connector-jdbc May 29, 2024
@eskabetxe eskabetxe changed the title [WIP][FLINK-35363] FLIP-449: Reorganization of flink-connector-jdbc [FLINK-35363] FLIP-449: Reorganization of flink-connector-jdbc May 29, 2024
@eskabetxe eskabetxe marked this pull request as draft May 29, 2024 19:10
@eskabetxe eskabetxe force-pushed the FLINK-35363 branch 5 times, most recently from c5ad76f to 91578cd Compare May 29, 2024 20:42
@eskabetxe eskabetxe force-pushed the FLINK-35363 branch 18 times, most recently from 8a57c6d to efd79a2 Compare June 3, 2024 12:47
RocMarshal
RocMarshal reviewed Jul 12, 2024
View reviewed changes
Copy link
Contributor

@RocMarshal RocMarshal left a comment

Choose a reason for hiding this comment

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

Thanks @eskabetxe for the hard work!
Looks nice to me on the functional refactor.
Just left a few of comments about doc/naming.
Please let me know what's your opinion :)

flink-connector-jdbc/src/main/java/org/apache/flink/connector/jdbc/dialect/AbstractDialect.java Outdated Show resolved Hide resolved
...onnector-jdbc/src/main/java/org/apache/flink/connector/jdbc/catalog/AbstractJdbcCatalog.java Outdated Show resolved Hide resolved
...r-jdbc/src/main/java/org/apache/flink/connector/jdbc/converter/AbstractJdbcRowConverter.java Outdated Show resolved Hide resolved
...ain/java/org/apache/flink/connector/jdbc/internal/options/InternalJdbcConnectionOptions.java Outdated Show resolved Hide resolved
...-connector-jdbc-core/src/main/java/org/apache/flink/connector/jdbc/core/datastream/Jdbc.java
import org.apache.flink.connector.jdbc.core.datastream.source.JdbcSource;
import org.apache.flink.connector.jdbc.core.datastream.source.JdbcSourceBuilder;

/** Facade to create JDBC stream sources and sinks. */
Copy link
Contributor

Choose a reason for hiding this comment

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

The repo is lacking of documentation pages to describe the new jdbc sink2 and the current usage for here.
Would you like to supplement it by https://issues.apache.org/jira/browse/FLINK-35811 ?

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 think that all doc should also be refactored, maybe have one sub-page per connector, and clean all deprecated docs.
We could start talk about it, but it is on the plan.

...nector-jdbc/src/main/java/org/apache/flink/connector/jdbc/dialect/JdbcDialectTypeMapper.java Outdated Show resolved Hide resolved
...connector-jdbc/src/main/java/org/apache/flink/connector/jdbc/converter/JdbcRowConverter.java Outdated Show resolved Hide resolved
flink-connector-jdbc/src/main/java/org/apache/flink/connector/jdbc/source/JdbcSource.java Outdated Show resolved Hide resolved
...k-connector-jdbc/src/main/java/org/apache/flink/connector/jdbc/source/JdbcSourceBuilder.java Outdated Show resolved Hide resolved
...c/src/main/java/org/apache/flink/connector/jdbc/source/reader/extractor/ResultExtractor.java Outdated Show resolved Hide resolved
@eskabetxe eskabetxe force-pushed the FLINK-35363 branch 6 times, most recently from 4724698 to d9cb6ca Compare July 12, 2024 21:08
@eskabetxe
Copy link
Member Author

eskabetxe commented Jul 12, 2024 •
edited
Loading

@leonardBang @1996fanrui @snuyanzin can you check please when you have time

70% of the refactor has been completed.
The deprecation of the old sources and sinks is still pending, as they are still used in the table API.
The idea is to review and merge this part (there are already significant changes) and continue working on another PR to:

  • Modify the table API to use the new sources and sinks.
  • Deprecate and move the old sources and sinks.
  • Finish organizing the code in the core module.
  • Reorganize the documentation.

Note: Before the merge, we can consider removing the backward-compatibility module (and workflow) to avoid generating the jar, as it was only meant to be a helper.

@eskabetxe eskabetxe marked this pull request as ready for review July 12, 2024 21:40
@RocMarshal
Copy link
Contributor

Note: Before the merge, we can consider removing the backward-compatibility module (and workflow) to avoid generating the jar, as it was only meant to be a helper.

Hi, @eskabetxe
Considering that this is a relatively large refactoring action,
Perhaps retaining this module can also help developers quickly verify and fix compatibility related issues.
How about continuing to keep one or two versions of the cycle? In other words, keep this module until the refactored code becomes stable and mature.
Please let me know what's your opinion.

RocMarshal
RocMarshal approved these changes Jul 23, 2024
View reviewed changes
Copy link
Contributor

@RocMarshal RocMarshal left a comment

Choose a reason for hiding this comment

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

Thanks @eskabetxe for the hard work.
LGTM +1.
now ping @1996fanrui

@1996fanrui 1996fanrui self-assigned this Jul 23, 2024
@1996fanrui
Copy link
Member

Thanks @eskabetxe for the contribution, and thanks @RocMarshal for the review!

Due to both of you are active contributor of flink-connector-jdbc, and you aren't committer. So I will merge this PR if no objection before next Monday(in 5 days).

@1996fanrui 1996fanrui merged commit 39ba942 into apache:main Jul 29, 2024
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RocMarshal RocMarshal RocMarshal approved these changes

Assignees

@1996fanrui 1996fanrui

Labels
component=BuildSystem component=Connectors/JDBC
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@eskabetxe @RocMarshal @1996fanrui