Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BEAM-12044] JdbcIO read: always force autocommit to false #14338

Closed
wants to merge 1 commit into from
+3 −0
Closed

[BEAM-12044] JdbcIO read: always force autocommit to false #14338

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions sdks/java/io/jdbc/src/main/java/org/apache/beam/sdk/io/jdbc/JdbcIO.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -903,6 +903,9 @@ public void processElement(ProcessContext context) throws Exception {
if (connection == null) {
connection = dataSource.getConnection();
}
// PostgreSQL requires autocommit to be disabled to enable cursor streaming
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

drive-by: could you include the Postgres documentation link in this comment?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean the comment in the source code or the comment of the commit?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the source code. If someone wonders why this is here and why Postgres requires this (and whether other DBs might also require it for the same reason), it's better if they can see it right there in the code rather than have to dig into Git history.

I would perhaps propose the following phrasing:

Some databases require autocommit to be disabled to enable cursor streaming.
E.g. PostgreSQL: https://jdbc.postgresql.org/documentation/head/query.html#query-with-cursor

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done meanwhile ;)

Copy link
Contributor

@aromanenko-dev aromanenko-dev Mar 25, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd suggest to set it to false only if it was not explicitly set to true by connection properties configuration. Otherwise, it won't be possible to override and it can be confusing for users (since we already provide a way to do that with DataSourceConfiguration).

Copy link
Contributor Author

@turb turb Mar 25, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So we would store the configparameter passed in withDataSourceConfiguration? What would be the path to retrieve this parameter from it?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, I guess it won't be so easy to check without additional work (please, correct me if I'm wrong).

My main concern is the following - if we provide a way to configure IO in some way, we don't have to silently override it without taking into account the user config options if they were set explicitly.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I have an idea on how the parameter can be specified, it will be additional work yes, but I can spend some reasonable time on it.

However, I am inclined to invoke YAGNI principle: once this parameter is needed (and I can't determine how someone would need autocommit while reading), the way to parameterize it and to take it into account in JdbcIO will appear more clearly.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, since we have an agreement in general, let's keep this behaviour but, please, add a log message that autoCommit was forced to true and, as I mentioned before, create PR from a feature branch. Thanks!

// see https://jdbc.postgresql.org/documentation/head/query.html#query-with-cursor
connection.setAutoCommit(false);
try (PreparedStatement statement =
connection.prepareStatement(
query.get(), ResultSet.TYPE_FORWARD_ONLY, ResultSet.CONCUR_READ_ONLY)) {
Expand Down