-
Notifications
You must be signed in to change notification settings - Fork 13k
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-31545][jdbc-driver] Create executor in flink connection #22289
[FLINK-31545][jdbc-driver] Create executor in flink connection #22289
Conversation
bdc83df
to
da78097
Compare
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.
@FangYongs Thanks for your contribution, I've left my comments.
...k-table/flink-sql-jdbc-driver/src/main/java/org/apache/flink/table/jdbc/FlinkConnection.java
Show resolved
Hide resolved
...k-table/flink-sql-jdbc-driver/src/main/java/org/apache/flink/table/jdbc/FlinkConnection.java
Show resolved
Hide resolved
...ble/flink-sql-jdbc-driver/src/test/java/org/apache/flink/table/jdbc/FlinkConnectionTest.java
Show resolved
Hide resolved
da78097
to
20641fa
Compare
Thanks @libenchao , I have rebased master and updated this PR |
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.
@FangYongs Thanks for the updating, it looks good to me. I only left one nit comment.
|
||
public FlinkConnection(DriverUri driverUri) { | ||
this.driverUri = driverUri; | ||
// TODO Support default context from map to get gid of flink core for jdbc driver |
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.
It would be great if you also add the JIra link in the 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.
Thanks @libenchao , added the jira link
CI now fails due to the API compatibility problem, could you fix that. |
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.
+1, @FangYongs Thanks for your contribution!
What is the purpose of the change
This PR aims to create
Executor
inFlinkConnection
and supports catalog/schema related operationsBrief change log
Executor
inFlinkConnection
FlinkConnection
Verifying this change
This change added tests and can be verified as follows:
testCatalogSchema
andtestClientInfo
inFlinkConnectionTest
Does this pull request potentially affect one of the following parts:
@Public(Evolving)
: (yes / no) noDocumentation