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
