[JdbcIO] Adding disableAutoCommit flag #32988
Conversation
cwashcraft
force-pushed
the
master
branch
3 times, most recently
from
d9dd509
to
ccd1b57
Compare
|
fixes #31111 |
|
assign set of reviewers |
|
Assigning reviewers. If you would like to opt out of this review, comment R: @m-trieu for label java. Available commands:
The PR bot will only process comments in the main thread (not review comments). |
|
assign set of reviewers |
| // if calling setAutoCommit on a non-logged database | ||
| if (disableAutoCommit) { | ||
| LOG.info("Autocommit has been disabled"); | ||
| connection.setAutoCommit(false); |
There was a problem hiding this comment.
It's not recommended to mutate connection here. Connection is cached once created.
For #31111 one can set "withDataSourceConfiguration" or "withDataSourceProviderFn" which creates DataSource that disabled autocommit, no need additional parameters or SDK change here
There was a problem hiding this comment.
While I agree with you, this setAutoCommit statement has already been present in the code for some time. This PR is only making it conditional. Removing it entirely would bring a breaking change.
There was a problem hiding this comment.
I see, I understand now, it is this existing "setAutoCommit" call causing issues for certain jdbc provider. Yeah the existing code is not idea
|
Does this also affect Jdbc write where it always set "setAutoCommit" also |
| @@ -799,6 +810,11 @@ public ReadRows withOutputParallelization(boolean outputParallelization) { | |||
| return toBuilder().setOutputParallelization(outputParallelization).build(); | |||
| } | |||
|
|
|||
| /** Whether to disable auto commit on read. Defaults to true if not provided. */ | |||
There was a problem hiding this comment.
Could you please elaborate javadoc. For example, noting that some Jdbc provider does not support setAutoCommit, which is the circumstance one needs to set this to false
* unit tests * CHANGES.md * unit tests * CHANGES.md updating getDisableAutoCommit variable naming cleanup cleanup cleanup cleanup cleanup whitespace cleanup whitespace cleanup
Addresses #31111
Adding flag to conditionally disable auto-commit for JdbcIO read due to current incompatibility with Informix database.
Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
addresses #123), if applicable. This will automatically add a link to the pull request in the issue. If you would like the issue to automatically close on merging the pull request, commentfixes #<ISSUE NUMBER>instead.CHANGES.mdwith noteworthy changes.See the Contributor Guide for more tips on how to make review process smoother.
To check the build health, please visit https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md
GitHub Actions Tests Status (on master branch)
See CI.md for more information about GitHub Actions CI or the workflows README to see a list of phrases to trigger workflows.