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/18 04:44:01 UTC
[PR] [FLINK-33460][Connector/JDBC] Support property authentication connection. [flink-connector-jdbc]
RocMarshal opened a new pull request, #115:
URL: https://github.com/apache/flink-connector-jdbc/pull/115
- Support property authentication connection.
--
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-33460][Connector/JDBC] Support property authentication connection. [flink-connector-jdbc]
Posted by "RocMarshal (via GitHub)" <gi...@apache.org>.
RocMarshal commented on code in PR #115:
URL: https://github.com/apache/flink-connector-jdbc/pull/115#discussion_r1572398050
##########
flink-connector-jdbc/src/main/java/org/apache/flink/connector/jdbc/JdbcConnectionOptions.java:
##########
@@ -90,13 +98,31 @@ public JdbcConnectionOptionsBuilder withDriverName(String driverName) {
return this;
}
+ public JdbcConnectionOptionsBuilder withProperties(Properties properties) {
Review Comment:
Thanks @eskabetxe for the comments.
That sounds nice to me
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-33460][Connector/JDBC] Support property authentication connection. [flink-connector-jdbc]
Posted by "RocMarshal (via GitHub)" <gi...@apache.org>.
RocMarshal commented on code in PR #115:
URL: https://github.com/apache/flink-connector-jdbc/pull/115#discussion_r1572083767
##########
flink-connector-jdbc/src/main/java/org/apache/flink/connector/jdbc/JdbcConnectionOptions.java:
##########
@@ -90,13 +98,31 @@ public JdbcConnectionOptionsBuilder withDriverName(String driverName) {
return this;
}
+ public JdbcConnectionOptionsBuilder withProperties(Properties properties) {
Review Comment:
hi, @eskabetxe Thanks a lot for your quick-review 😄 .
```
.withProperty("a", "a1")
.withProperties(new properties())
.withProperty("b", "b1")
```
> .....
> that a and b should be in the properties, or just b...
It's right.
It's difficult to express , which depends the what cases devs need and I have to admit that the lines referenced above is ambiguous now.
I have come up with two temporary ways to alleviate this ambiguity:
- Keep the current logic, and then we add comment to describe semantics for these two methods.
OR
- Add a flag parameter to method `withProperties(new properties(), true/false)` , indicating whether to force assign entire `properties`, rather than a part of the `properties` by `upsert` mode.
Of course, I'd appreciated to hear any ideas form you / community devs ! 😃
Please let me know what's your opinion.
--
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-33460][Connector/JDBC] Support property authentication connection. [flink-connector-jdbc]
Posted by "eskabetxe (via GitHub)" <gi...@apache.org>.
eskabetxe commented on code in PR #115:
URL: https://github.com/apache/flink-connector-jdbc/pull/115#discussion_r1572010803
##########
flink-connector-jdbc/src/main/java/org/apache/flink/connector/jdbc/JdbcConnectionOptions.java:
##########
@@ -90,13 +98,31 @@ public JdbcConnectionOptionsBuilder withDriverName(String driverName) {
return this;
}
+ public JdbcConnectionOptionsBuilder withProperties(Properties properties) {
Review Comment:
I have mixed feelings about this method...
I understand that it may be practical, but as a user I can expect that the properties I am passing will override all properties already defined.
as an example:
```
.withProperty("a", "a1")
.withProperties(new properties())
.withProperty("b", "b1")
```
If you saw that (without knowing the implementation), what will you expect.
that a and b should be in the properties, or just b...
--
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-33460][Connector/JDBC] Support property authentication connection. [flink-connector-jdbc]
Posted by "1996fanrui (via GitHub)" <gi...@apache.org>.
1996fanrui merged PR #115:
URL: 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-33460][Connector/JDBC] Support property authentication connection. [flink-connector-jdbc]
Posted by "RocMarshal (via GitHub)" <gi...@apache.org>.
RocMarshal commented on PR #115:
URL: https://github.com/apache/flink-connector-jdbc/pull/115#issuecomment-2068437230
hi, @Jiabao-Sun master, Could you help to have a review 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-33460][Connector/JDBC] Support property authentication connection. [flink-connector-jdbc]
Posted by "RocMarshal (via GitHub)" <gi...@apache.org>.
RocMarshal commented on code in PR #115:
URL: https://github.com/apache/flink-connector-jdbc/pull/115#discussion_r1572398050
##########
flink-connector-jdbc/src/main/java/org/apache/flink/connector/jdbc/JdbcConnectionOptions.java:
##########
@@ -90,13 +98,31 @@ public JdbcConnectionOptionsBuilder withDriverName(String driverName) {
return this;
}
+ public JdbcConnectionOptionsBuilder withProperties(Properties properties) {
Review Comment:
Thanks for the comments.
That sounds nice to me
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-33460][Connector/JDBC] Support property authentication connection. [flink-connector-jdbc]
Posted by "RocMarshal (via GitHub)" <gi...@apache.org>.
RocMarshal commented on PR #115:
URL: https://github.com/apache/flink-connector-jdbc/pull/115#issuecomment-2063308586
hi, @snuyanzin @eskabetxe Could you help to have a review 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-33460][Connector/JDBC] Support property authentication connection. [flink-connector-jdbc]
Posted by "RocMarshal (via GitHub)" <gi...@apache.org>.
RocMarshal commented on code in PR #115:
URL: https://github.com/apache/flink-connector-jdbc/pull/115#discussion_r1572083767
##########
flink-connector-jdbc/src/main/java/org/apache/flink/connector/jdbc/JdbcConnectionOptions.java:
##########
@@ -90,13 +98,31 @@ public JdbcConnectionOptionsBuilder withDriverName(String driverName) {
return this;
}
+ public JdbcConnectionOptionsBuilder withProperties(Properties properties) {
Review Comment:
hi, @eskabetxe Thanks a lot for your quick-review 😄 .
```
.withProperty("a", "a1")
.withProperties(new properties())
.withProperty("b", "b1")
```
> .....
> that a and b should be in the properties, or just b...
It's right.
It's difficult to express , which depends the what cases devs need and I have to admit that the lines referenced above is ambiguous now.
I have come up with two temporary ways to alleviate this ambiguity:
- Keep the current logic, and then we add comment to describe semantics for these two methods.
OR
- Add a flag parameter to method `withProperties(new properties(), true/false)` , indicating whether to force assign entire `properties`, rather than a part of the `properties` by `upsert` mode.
Of course, I'd appreciated to hear any ideas form you / community devs ! 😃
##########
flink-connector-jdbc/src/main/java/org/apache/flink/connector/jdbc/JdbcConnectionOptions.java:
##########
@@ -90,13 +98,31 @@ public JdbcConnectionOptionsBuilder withDriverName(String driverName) {
return this;
}
+ public JdbcConnectionOptionsBuilder withProperties(Properties properties) {
Review Comment:
hi, @eskabetxe Thanks a lot for your quick-review 😄 .
```
.withProperty("a", "a1")
.withProperties(new properties())
.withProperty("b", "b1")
```
> .....
> that a and b should be in the properties, or just b...
It's right.
It's difficult to express , which depends the what cases devs need and I have to admit that the lines referenced above is ambiguous now.
I have come up with two temporary ways to alleviate this ambiguity:
- Keep the current logic, and then we add comment to describe semantics for these two methods.
OR
- Add a flag parameter to method `withProperties(new properties(), true/false)` , indicating whether to force assign entire `properties`, rather than a part of the `properties` by `upsert` mode.
Of course, I'd appreciated to hear any ideas form you / community devs ! 😃
--
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-33460][Connector/JDBC] Support property authentication connection. [flink-connector-jdbc]
Posted by "eskabetxe (via GitHub)" <gi...@apache.org>.
eskabetxe commented on code in PR #115:
URL: https://github.com/apache/flink-connector-jdbc/pull/115#discussion_r1571058904
##########
flink-connector-jdbc/src/main/java/org/apache/flink/connector/jdbc/JdbcConnectionOptions.java:
##########
@@ -90,6 +101,11 @@ public JdbcConnectionOptionsBuilder withDriverName(String driverName) {
return this;
}
+ public JdbcConnectionOptionsBuilder withExtendProperties(Properties extendProperties) {
Review Comment:
won't be better have the method to allow add one property
```
private Properties properties = new Properties();
public JdbcConnectionOptionsBuilder withConfig(String config, String value) {
this.properties.put(config, value);
return this;
}
```
this way we could also start thinking on moving user, pass and driver to this, as they will be used always inside the properties..
simplifying some logic in [SimpleJdbcConnectionProvider.java](https://github.com/apache/flink-connector-jdbc/pull/115/files#diff-1a261021e67f437344c5b6b2b5da274ff6914c51d8b5163ef30f03e6ba3614c5)
```
public JdbcConnectionOptionsBuilder withUsername(String username) {
this.properties.put("user", username);
return this;
}
```
--
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-33460][Connector/JDBC] Support property authentication connection. [flink-connector-jdbc]
Posted by "eskabetxe (via GitHub)" <gi...@apache.org>.
eskabetxe commented on code in PR #115:
URL: https://github.com/apache/flink-connector-jdbc/pull/115#discussion_r1572162247
##########
flink-connector-jdbc/src/main/java/org/apache/flink/connector/jdbc/JdbcConnectionOptions.java:
##########
@@ -90,13 +98,31 @@ public JdbcConnectionOptionsBuilder withDriverName(String driverName) {
return this;
}
+ public JdbcConnectionOptionsBuilder withProperties(Properties properties) {
Review Comment:
@RocMarshal I would say the easiest way, eliminating the method.
If someone has a properties, they can do:
`properties.forEach((key, value) -> builder.withProperty(key, value));`
Under the hood, the `properties.putAll` is what it does.
I always prefer to keep it simple..
If we have to explain, document or add something to explain what is being done, we are doing it 'wrong'.
--
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-33460][Connector/JDBC] Support property authentication connection. [flink-connector-jdbc]
Posted by "eskabetxe (via GitHub)" <gi...@apache.org>.
eskabetxe commented on code in PR #115:
URL: https://github.com/apache/flink-connector-jdbc/pull/115#discussion_r1572162247
##########
flink-connector-jdbc/src/main/java/org/apache/flink/connector/jdbc/JdbcConnectionOptions.java:
##########
@@ -90,13 +98,31 @@ public JdbcConnectionOptionsBuilder withDriverName(String driverName) {
return this;
}
+ public JdbcConnectionOptionsBuilder withProperties(Properties properties) {
Review Comment:
@RocMarshal I would say the easiest way, eliminating the method.
If someone has a properties, they can do:
`properties.forEach((key, value) -> builder.withProperty(key, value));`
Under the hood, the `properties.putAll` is what it does.
--
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-33460][Connector/JDBC] Support property authentication connection. [flink-connector-jdbc]
Posted by "RocMarshal (via GitHub)" <gi...@apache.org>.
RocMarshal commented on code in PR #115:
URL: https://github.com/apache/flink-connector-jdbc/pull/115#discussion_r1571809638
##########
flink-connector-jdbc/src/main/java/org/apache/flink/connector/jdbc/JdbcConnectionOptions.java:
##########
@@ -90,6 +101,11 @@ public JdbcConnectionOptionsBuilder withDriverName(String driverName) {
return this;
}
+ public JdbcConnectionOptionsBuilder withExtendProperties(Properties extendProperties) {
Review Comment:
@eskabetxe Thanks for your quick-review.
The comments sounds good to me !
And I updated it based on your comments.
PTAL~ thank you very much : )
--
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