Re: [PR] [FLINK-33460][Connector/JDBC] Support property authentication connection. [flink-connector-jdbc]
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-unsubscr...@flink.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [FLINK-33460][Connector/JDBC] Support property authentication connection. [flink-connector-jdbc]
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-unsubscr...@flink.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [FLINK-33460][Connector/JDBC] Support property authentication connection. [flink-connector-jdbc]
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-unsubscr...@flink.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] [FLINK-33460][Connector/JDBC] Support property authentication connection. [flink-connector-jdbc]
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-unsubscr...@flink.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] [FLINK-33460][Connector/JDBC] Support property authentication connection. [flink-connector-jdbc]
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-unsubscr...@flink.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] [FLINK-33460][Connector/JDBC] Support property authentication connection. [flink-connector-jdbc]
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-unsubscr...@flink.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] [FLINK-33460][Connector/JDBC] Support property authentication connection. [flink-connector-jdbc]
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-unsubscr...@flink.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] [FLINK-33460][Connector/JDBC] Support property authentication connection. [flink-connector-jdbc]
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-unsubscr...@flink.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] [FLINK-33460][Connector/JDBC] Support property authentication connection. [flink-connector-jdbc]
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-unsubscr...@flink.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] [FLINK-33460][Connector/JDBC] Support property authentication connection. [flink-connector-jdbc]
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-unsubscr...@flink.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] [FLINK-33460][Connector/JDBC] Support property authentication connection. [flink-connector-jdbc]
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-unsubscr...@flink.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] [FLINK-33460][Connector/JDBC] Support property authentication connection. [flink-connector-jdbc]
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-unsubscr...@flink.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[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-unsubscr...@flink.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
