-
Notifications
You must be signed in to change notification settings - Fork 203
[FLINK-38733] Add new SplitterEnumerator on JdbcSource #180
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
base: main
Are you sure you want to change the base?
Conversation
2d3c1ac to
35399ed
Compare
35399ed to
59dea79
Compare
|
@ferenc-csaky can you have a look when have free time.. |
ferenc-csaky
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did not check the implementation details yet, added some interface-related comments.
| import java.util.function.Supplier; | ||
|
|
||
| /** Interface for jdbc sql split enumerator. */ | ||
| public interface SplitterEnumerator extends AutoCloseable, Serializable { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this IF be considered as a public API? If yes, lets add @PublicEvolving, otherwise @Internal. Based on JdbcSqlSplitEnumeratorBase, I guess @PublicEvolving. And that also means we should add javadoc to the functions. As far as I see most of them can be applied from JdbcSqlSplitEnumeratorBase.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
correct this should be PublicEvolving, the implementation can also be PublicEvolving if we decide to deprecate the JdbcParameterValuesProvider
...ache/flink/connector/jdbc/core/datastream/source/enumerator/splitter/SplitterEnumerator.java
Outdated
Show resolved
Hide resolved
...nk/connector/jdbc/core/datastream/source/enumerator/splitter/PreparedSplitterEnumerator.java
Outdated
Show resolved
Hide resolved
| @PublicEvolving | ||
| public abstract class JdbcSqlSplitEnumeratorBase<SplitT> implements AutoCloseable, Serializable { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this is a public API, we cannot throw it away like this, we need to deprecate it first and keep the related config methods (deprecated) as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think about this, and I don't deprecated it because it is only used internally and cannot be passed in any way to source.. could have a wrong annotation?
...ache/flink/connector/jdbc/core/datastream/source/enumerator/splitter/SplitterEnumerator.java
Outdated
Show resolved
Hide resolved
e422dbb to
cdbee4f
Compare
cdbee4f to
6964598
Compare
This pull request refactors the JDBC source split enumeration logic in the Flink connector, replacing the previous
SqlTemplateSplitEnumeratorand related classes with a new abstraction calledSplitterEnumerator. The builder and enumerator classes are updated to use this new abstraction, simplifying split management and improving extensibility. The changes also update lineage handling and testing methods to use the new splitter abstraction.Split Enumeration Refactoring
SqlTemplateSplitEnumeratorandJdbcSqlSplitEnumeratorBasewith the newSplitterEnumeratorabstraction acrossJdbcSource,JdbcSourceBuilder, andJdbcSourceEnumerator, updating constructors, fields, and method calls accordingly. [1] [2] [3] [4]SplitterEnumerator, and added logic to select the appropriate splitter implementation based on provided parameters. [1] [2] [3]Lineage and Testing Updates
SqlTemplateSplitEnumerator. [1] [2]Miscellaneous