-
Notifications
You must be signed in to change notification settings - Fork 177
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-35176][Connector/JDBC] Support property authentication connection for JDBC catalog & dynamic table #116
Conversation
RocMarshal
commented
Apr 19, 2024
- Support property authentication connection for JDBC catalog & dynamic table
blocked by #115 |
HI, @caicancai @GOODBOY008 Could you help have a review if you had the free time ? thx a lot. |
flink-connector-jdbc/src/main/java/org/apache/flink/connector/jdbc/catalog/JdbcCatalog.java
Show resolved
Hide resolved
flink-connector-jdbc/src/main/java/org/apache/flink/connector/jdbc/catalog/JdbcCatalog.java
Show resolved
Hide resolved
flink-connector-jdbc/src/main/java/org/apache/flink/connector/jdbc/catalog/JdbcCatalog.java
Show resolved
Hide resolved
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.
LGTM
Thanks @caicancai for the review. |
Hi, @eskabetxe Would you mind having take a look if you had the free time ? Many thx! |
this.baseUrl = baseUrl.endsWith("/") ? baseUrl : baseUrl + "/"; | ||
this.defaultUrl = this.baseUrl + defaultDatabase; | ||
this.connectionProperties = Preconditions.checkNotNull(connectionProperties); | ||
if (StringUtils.isNullOrWhitespaceOnly(connectionProperties.getProperty(USER_KEY))) { |
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.
we should maintain the current checks
checkArgument(!StringUtils.isNullOrWhitespaceOnly(connectionProperties.getProperty(USER_KEY)));
checkArgument(!StringUtils.isNullOrWhitespaceOnly(connectionProperties.getProperty(PASSWORD_KEY)));
Thanks @eskabetxe for the comments. Updated. |
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.
LGTM
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 @RocMarshal for the contribution, and @eskabetxe @caicancai for the review!
Merging~
…ion for JDBC catalog
…ion for dynamic jdbc source table