dforciea commented on a change in pull request #13570:
URL: https://github.com/apache/flink/pull/13570#discussion_r502798851
##########
File path:
flink-connectors/flink-connector-jdbc/src/main/java/org/apache/flink/connector/jdbc/table/JdbcDynamicTableFactory.java
##########
@@ -109,6 +109,11 @@
.withDescription("gives the reader a hint as to the number of
rows that should be fetched, from" +
" the database when reading per round trip. If the
value specified is zero, then the hint is ignored. The" +
" default value is zero.");
+ private static final ConfigOption<Boolean> SCAN_AUTO_COMMIT =
ConfigOptions
+ .key("scan.auto-commit")
+ .booleanType()
+ .noDefaultValue()
Review comment:
I had let it be optional so that it could use the JDBC default. But, per
the JDBC spec the default is true, so your suggestion makes sense. I'll make
that change.
In the email thread, you had made a comment about setting auto commit to
false by default within `JdbcRowDataInputFormat.Builder` in the 1.11 branch so
that it would do the right thing for scans in postgres. However, that would
conflict with setting the default to be true here. If someone expected that
behavior in a 1.11 patch and then we add this, the behavior would revert back
if they don't specify this option explicitly.
In general, I think that leaving auto commit enabled is the right thing to
do. However, this is a config option specifically for doing scans, so I could
see an argument for letting the default be false in this case. But, I can also
see a good argument for making it match the JDBC driver default. I would
suggest whatever is decided that it should be consistent, so if we set the
default to be true here we probably shouldn't change
`JdbcRowDataInputFormat.Builder`.
Let me know which direction you'd like to go with 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.
For queries about this service, please contact Infrastructure at:
[email protected]
