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-33463][Connector/JDBC] Support the implementation of dynamic source tables based on the new source #117

Merged
merged 2 commits into from
May 9, 2024
Merged

[FLINK-33463][Connector/JDBC] Support the implementation of dynamic source tables based on the new source #117

merged 2 commits into from
May 9, 2024

Conversation

RocMarshal
Copy link
Contributor

  • Support the implementation of dynamic source tables based on the new source

@RocMarshal RocMarshal force-pushed the FLINK-33463 branch 7 times, most recently from a9ba994 to 645758b Compare April 20, 2024 13:49
@RocMarshal RocMarshal closed this Apr 20, 2024
@RocMarshal RocMarshal reopened this Apr 20, 2024
@RocMarshal RocMarshal marked this pull request as ready for review April 23, 2024 10:28
@RocMarshal
Copy link
Contributor Author

blocked by #116

@RocMarshal
Copy link
Contributor Author

HI, @eskabetxe @caicancai Could you help have a review if you had the free time ? thx a lot.

@RocMarshal RocMarshal closed this Apr 26, 2024
@RocMarshal RocMarshal reopened this Apr 26, 2024
@RocMarshal RocMarshal force-pushed the FLINK-33463 branch 2 times, most recently from 01a36e8 to cda6098 Compare May 7, 2024 02:57
@RocMarshal RocMarshal force-pushed the FLINK-33463 branch 2 times, most recently from e4f5344 to db7d938 Compare May 7, 2024 10:03
RocMarshal
RocMarshal commented May 8, 2024
View reviewed changes
Copy link
Contributor Author

@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 review.
I made some change based on your comments.
PTAL if you had the free time~ :)

@RocMarshal RocMarshal requested a review from eskabetxe May 8, 2024 06:02
eskabetxe
eskabetxe approved these changes May 8, 2024
View reviewed changes
Copy link
Member

@eskabetxe eskabetxe left a comment

Choose a reason for hiding this comment

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

LGTM

RocMarshal
RocMarshal commented May 8, 2024
View reviewed changes
Copy link
Contributor Author

@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 quick review~
now ping @1996fanrui

1996fanrui
1996fanrui approved these changes May 9, 2024
View reviewed changes
Copy link
Member

@1996fanrui 1996fanrui left a comment

Choose a reason for hiding this comment

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

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

LGTM

@1996fanrui 1996fanrui merged commit bde28e6 into apache:main May 9, 2024
12 checks passed
@RocMarshal
Copy link
Contributor Author

RocMarshal commented May 30, 2024 •
edited
Loading

Hi, @1996fanrui .
Kafka introduced KafkaSource API in the Flink 1.12 branch https://issues.apache.org/jira/browse/FLINK-18323
Afterwards, in Flink version 1.14, KafkaDynamicTableSource was implemented based on KafkaSource https://issues.apache.org/jira/browse/FLINK-22914

So, Maybe we should revert this commit and re-commit it in the next second or third version
Please let me know what's your opinion.
CC @eskabetxe

@1996fanrui
Copy link
Member

Maybe we should revert this commit and re-commit it in the next second or third version

Sounds make sense to me. The JdbcSource is introduced in the current version, it may be not stable. So it's better to consider it as the default Jdbc connector for SQL job after community think it's stable.

@RocMarshal RocMarshal mentioned this pull request May 31, 2024
Merged
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eskabetxe eskabetxe eskabetxe approved these changes

@1996fanrui 1996fanrui 1996fanrui approved these changes

Assignees
No one assigned
Labels
component=Connectors/JDBC
Projects
None yet
Milestone
No milestone
Development

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