-
Notifications
You must be signed in to change notification settings - Fork 5.4k
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.
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
Fix failure with enabling JDBC autocommit #23453
Conversation
Just a nit to add the PR number to release note entry.
|
1d94a47
to
f17a7e6
Compare
connection.setAutoCommit(false); | ||
try (Statement statement = connection.createStatement()) { | ||
statement.execute("CREATE TABLE test_commit (x bigint)"); | ||
try { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's hard to see in github, but the only thing that's changed about this test is that it's wrapped in an extra try so I could put a finally at the end to drop the table.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A neat trick is to take the url in the browser and add the query parameter ?w=1
, which will cause whitespaces to be ignored. e.g. https://github.com/prestodb/presto/pull/23453/files?w=1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, one little nit.
presto-jdbc/src/test/java/com/facebook/presto/jdbc/TestJdbcConnection.java
Outdated
Show resolved
Hide resolved
There was a bug where enabling autocommit when it had previously been false would cause failure like: java.sql.SQLException: Connection is in auto-commit mode This was due to calling commit() from setAutoCommit() after autocommit had already been changed to true. Fixes prestodb#15916
f17a7e6
to
38d8a5d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the quick fix.
Description
There was a bug where enabling autocommit when it had previously been false would cause failure like:
java.sql.SQLException: Connection is in auto-commit mode
This was due to calling commit() from setAutoCommit() after autocommit had already been changed to true.
Motivation and Context
Fixes #15916
Impact
Fixes a failure when using JDBC when setting AutoCommit from false to true
Test Plan
unit test
Contributor checklist
Release Notes
Please follow release notes guidelines and fill in the release notes below.