-
Notifications
You must be signed in to change notification settings - Fork 205
[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
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.
...ache/flink/connector/jdbc/core/datastream/source/enumerator/splitter/SplitterEnumerator.java
Show resolved
Hide resolved
...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?
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 have rechecked the reason why this annotation was added at the time:
- A:The
org.apache.flink.connector.jdbc.core.datastream.source.JdbcSourceis annotated as@PublicEvolving- But the constructor methed of
JdbcSourcevisible scope is default withoutpublickeyword, although It's a bit odd.
- But the constructor methed of
- B:Therefore, not only the classes directly exposed in JdbcBuilder, but also the classes exposed in the constructors of the Jdbc class should be annotated as
PublicEvolving, that is,JdbcSqlSplitEnumeratorBaseis annotated withPublicEvolvingis correct.
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.
Therefore, overall, this suggestion requires our careful consideration.
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.
So what is the recommendation? Keep the class and have two constructors on JdbcSource one of them deprecated?
As you say is a little odd the use today, as constructor is only accessible by Builder and there is the only place we use it..
For example the current lineage expects an SqlTemplateSplitEnumerator not and JdbcSqlSplitEnumeratorBase..
I will try to rethink this..
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.
So what is the recommendation? Keep the class and have two constructors on JdbcSource one of them deprecated?
As you say is a little odd the use today, as constructor is only accessible by Builder and there is the only place we use it.. For example the current lineage expects an SqlTemplateSplitEnumerator not and JdbcSqlSplitEnumeratorBase..
I will try to rethink this..
@eskabetxe
Sorry for not being able to devise a more elegant way to handle this transition.
-
I prefer that we should at least keep this class for a while, so that users who are already relying on it have time to migrate and adapt.
-
If we remove this class and its related components directly, it will certainly result in a poor experience for our users. If it must be removed, it would be best to include some release notes or documentation explaining the reasons behind this necessary change.
In contrast, the impact of this change on the SQL side is negligible.
Hope you have already thought of a better way to move forward.
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.
@RocMarshal @ferenc-csaky
Can you check again, I came with a new approach to keep this class during the transition..
...ache/flink/connector/jdbc/core/datastream/source/enumerator/splitter/SplitterEnumerator.java
Outdated
Show resolved
Hide resolved
cdbee4f to
6964598
Compare
6964598 to
63ad89f
Compare
5e14d45 to
63a2301
Compare
63a2301 to
4b0c038
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