You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by "RocMarshal (via GitHub)" <gi...@apache.org> on 2024/04/19 15:39:38 UTC
[PR] [FLINK-33460][Connector/JDBC] Support property authentication connection for JDBC catalog & dynamic table [flink-connector-jdbc]
RocMarshal opened a new pull request, #116:
URL: https://github.com/apache/flink-connector-jdbc/pull/116
- Support property authentication connection for JDBC catalog & dynamic table
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-35176][Connector/JDBC] Support property authentication connection for JDBC catalog & dynamic table [flink-connector-jdbc]
Posted by "caicancai (via GitHub)" <gi...@apache.org>.
caicancai commented on code in PR #116:
URL: https://github.com/apache/flink-connector-jdbc/pull/116#discussion_r1581771380
##########
flink-connector-jdbc/src/main/java/org/apache/flink/connector/jdbc/catalog/JdbcCatalog.java:
##########
@@ -77,17 +81,42 @@ public JdbcCatalog(
String pwd,
String baseUrl,
String compatibleMode) {
- super(userClassLoader, catalogName, defaultDatabase, username, pwd, baseUrl);
+ this(
+ userClassLoader,
+ catalogName,
+ defaultDatabase,
+ baseUrl,
+ compatibleMode,
+ getBriefAuthProperties(username, pwd));
+ }
+
+ /**
+ * Creates a JdbcCatalog.
Review Comment:
I mean why the comment is Creates a JdbcCatalog, not Create a JdbcCatalog
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-35176][Connector/JDBC] Support property authentication connection for JDBC catalog & dynamic table [flink-connector-jdbc]
Posted by "RocMarshal (via GitHub)" <gi...@apache.org>.
RocMarshal commented on PR #116:
URL: https://github.com/apache/flink-connector-jdbc/pull/116#issuecomment-2077035421
HI, @caicancai @GOODBOY008 Could you help have a review if you had the free time ? thx a lot.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-35176][Connector/JDBC] Support property authentication connection for JDBC catalog & dynamic table [flink-connector-jdbc]
Posted by "RocMarshal (via GitHub)" <gi...@apache.org>.
RocMarshal closed pull request #116: [FLINK-35176][Connector/JDBC] Support property authentication connection for JDBC catalog & dynamic table
URL: https://github.com/apache/flink-connector-jdbc/pull/116
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-35176][Connector/JDBC] Support property authentication connection for JDBC catalog & dynamic table [flink-connector-jdbc]
Posted by "RocMarshal (via GitHub)" <gi...@apache.org>.
RocMarshal commented on PR #116:
URL: https://github.com/apache/flink-connector-jdbc/pull/116#issuecomment-2092246949
Thanks @eskabetxe for the comments. Updated.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-35176][Connector/JDBC] Support property authentication connection for JDBC catalog & dynamic table [flink-connector-jdbc]
Posted by "RocMarshal (via GitHub)" <gi...@apache.org>.
RocMarshal commented on PR #116:
URL: https://github.com/apache/flink-connector-jdbc/pull/116#issuecomment-2081320858
Hi, @eskabetxe Would you mind having take a look if you had the free time ? Many thx!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-35176][Connector/JDBC] Support property authentication connection for JDBC catalog & dynamic table [flink-connector-jdbc]
Posted by "RocMarshal (via GitHub)" <gi...@apache.org>.
RocMarshal commented on code in PR #116:
URL: https://github.com/apache/flink-connector-jdbc/pull/116#discussion_r1580440708
##########
flink-connector-jdbc/src/main/java/org/apache/flink/connector/jdbc/catalog/JdbcCatalog.java:
##########
@@ -29,18 +29,22 @@
import org.apache.flink.table.catalog.exceptions.TableNotExistException;
import java.util.List;
+import java.util.Properties;
+
+import static org.apache.flink.connector.jdbc.JdbcConnectionOptions.getBriefAuthProperties;
/** Catalogs for relational databases via JDBC. */
@PublicEvolving
public class JdbcCatalog extends AbstractJdbcCatalog {
private final AbstractJdbcCatalog internal;
+ @Deprecated
/**
* Creates a JdbcCatalog.
Review Comment:
Anchor: A
##########
flink-connector-jdbc/src/main/java/org/apache/flink/connector/jdbc/catalog/JdbcCatalog.java:
##########
@@ -77,17 +81,42 @@ public JdbcCatalog(
String pwd,
String baseUrl,
String compatibleMode) {
- super(userClassLoader, catalogName, defaultDatabase, username, pwd, baseUrl);
+ this(
+ userClassLoader,
+ catalogName,
+ defaultDatabase,
+ baseUrl,
+ compatibleMode,
+ getBriefAuthProperties(username, pwd));
+ }
+
+ /**
+ * Creates a JdbcCatalog.
Review Comment:
@caicancai Thanks for the comment.
I typed it in based on the style like 'Anchor: A'.
Maybe I get the wrong meaning from the comment. Would you mind clarifying more details ? many thx. :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-35176][Connector/JDBC] Support property authentication connection for JDBC catalog & dynamic table [flink-connector-jdbc]
Posted by "1996fanrui (via GitHub)" <gi...@apache.org>.
1996fanrui merged PR #116:
URL: https://github.com/apache/flink-connector-jdbc/pull/116
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-35176][Connector/JDBC] Support property authentication connection for JDBC catalog & dynamic table [flink-connector-jdbc]
Posted by "RocMarshal (via GitHub)" <gi...@apache.org>.
RocMarshal commented on PR #116:
URL: https://github.com/apache/flink-connector-jdbc/pull/116#issuecomment-2074135052
blocked by https://github.com/apache/flink-connector-jdbc/pull/115
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-35176][Connector/JDBC] Support property authentication connection for JDBC catalog & dynamic table [flink-connector-jdbc]
Posted by "RocMarshal (via GitHub)" <gi...@apache.org>.
RocMarshal commented on PR #116:
URL: https://github.com/apache/flink-connector-jdbc/pull/116#issuecomment-2080456998
Thanks @caicancai for the review.
Could @1996fanrui you help take a look if you had the free time ?
Thank you~
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-35176][Connector/JDBC] Support property authentication connection for JDBC catalog & dynamic table [flink-connector-jdbc]
Posted by "eskabetxe (via GitHub)" <gi...@apache.org>.
eskabetxe commented on code in PR #116:
URL: https://github.com/apache/flink-connector-jdbc/pull/116#discussion_r1584601921
##########
flink-connector-jdbc/src/main/java/org/apache/flink/connector/jdbc/catalog/AbstractJdbcCatalog.java:
##########
@@ -88,32 +92,49 @@ public abstract class AbstractJdbcCatalog extends AbstractCatalog {
private static final Logger LOG = LoggerFactory.getLogger(AbstractJdbcCatalog.class);
protected final ClassLoader userClassLoader;
- protected final String username;
- protected final String pwd;
protected final String baseUrl;
protected final String defaultUrl;
+ protected final Properties connectionProperties;
+ @Deprecated
public AbstractJdbcCatalog(
ClassLoader userClassLoader,
String catalogName,
String defaultDatabase,
String username,
String pwd,
String baseUrl) {
+ this(
+ userClassLoader,
+ catalogName,
+ defaultDatabase,
+ baseUrl,
+ getBriefAuthProperties(username, pwd));
+ }
+
+ public AbstractJdbcCatalog(
+ ClassLoader userClassLoader,
+ String catalogName,
+ String defaultDatabase,
+ String baseUrl,
+ Properties connectionProperties) {
super(catalogName, defaultDatabase);
checkNotNull(userClassLoader);
- checkArgument(!StringUtils.isNullOrWhitespaceOnly(username));
- checkArgument(!StringUtils.isNullOrWhitespaceOnly(pwd));
checkArgument(!StringUtils.isNullOrWhitespaceOnly(baseUrl));
JdbcCatalogUtils.validateJdbcUrl(baseUrl);
this.userClassLoader = userClassLoader;
- this.username = username;
- this.pwd = pwd;
this.baseUrl = baseUrl.endsWith("/") ? baseUrl : baseUrl + "/";
this.defaultUrl = this.baseUrl + defaultDatabase;
+ this.connectionProperties = Preconditions.checkNotNull(connectionProperties);
+ if (StringUtils.isNullOrWhitespaceOnly(connectionProperties.getProperty(USER_KEY))) {
Review Comment:
we should maintain the current checks
```
checkArgument(!StringUtils.isNullOrWhitespaceOnly(connectionProperties.getProperty(USER_KEY)));
checkArgument(!StringUtils.isNullOrWhitespaceOnly(connectionProperties.getProperty(PASSWORD_KEY)));
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-35176][Connector/JDBC] Support property authentication connection for JDBC catalog & dynamic table [flink-connector-jdbc]
Posted by "caicancai (via GitHub)" <gi...@apache.org>.
caicancai commented on code in PR #116:
URL: https://github.com/apache/flink-connector-jdbc/pull/116#discussion_r1580352031
##########
flink-connector-jdbc/src/main/java/org/apache/flink/connector/jdbc/catalog/JdbcCatalog.java:
##########
@@ -77,17 +81,42 @@ public JdbcCatalog(
String pwd,
String baseUrl,
String compatibleMode) {
- super(userClassLoader, catalogName, defaultDatabase, username, pwd, baseUrl);
+ this(
+ userClassLoader,
+ catalogName,
+ defaultDatabase,
+ baseUrl,
+ compatibleMode,
+ getBriefAuthProperties(username, pwd));
+ }
+
+ /**
+ * Creates a JdbcCatalog.
Review Comment:
creates？
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org