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.

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

Call commit() when auto-commit is enabled #1933

Conversation

nicoloboschi
Copy link
Contributor

Resolution for #1927

Copy link
Contributor

@katzyn katzyn left a comment

Choose a reason for hiding this comment

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

Thanks.

  1. I think we need a separate error code (org.h2.api.ErrorCode) and some unified localized message (org.h2.res.*, h2/src/docsrc/textbase/_messages_en.prop) like Method {0} is not allowed when connection is in auto-commit mode.

  2. It's very strange to disallow only the commit() method. At least no-arg rollback() should be disallowed too.

nicoloboschi pushed a commit to nicoloboschi/h2database that referenced this pull request May 28, 2019
@nicoloboschi nicoloboschi force-pushed the nicoloboschi/fix-commit-on-autocommit-false branch from 9fcb61d to 043f3c3 Compare May 28, 2019 11:31
@@ -42,6 +42,8 @@ public void test() throws Exception {
testSetInternalProperty();
testSetInternalPropertyToInitialValue();
testSetGetSchema();
testCommitOnAutoCommitSet();
testRollbackOnAutoCommitSet();
Copy link
Contributor

Choose a reason for hiding this comment

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

This all looks very good, but the tests aren't testing the non-default case right now, I think we need:

    SysProperties.FORCE_AUTOCOMMIT_OFF_ON_COMMIT = true
    try {
        testCommitOnAutoCommitSet();
        testRollbackOnAutoCommitSet();
    } finally {
                SysProperties.FORCE_AUTOCOMMIT_OFF_ON_COMMIT = false;
    }
    testCommitOnAutoCommitSet();
    testRollbackOnAutoCommitSet();

to test both ways.

Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

All SysProperties fields are "final", so I can't test it modifying FORCE_AUTOCOMMIT_OFF_ON_COMMIT value.
I cannot change the value using reflection because removing final modifier is not possible in JDK-12.
Aren't there any other cases like this in other tests?

Copy link
Contributor

Choose a reason for hiding this comment

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

This field is not strongly required to be final, but if you want to preserve this modifier, take a look on org.h2.test.unit.TestMemoryUnmapper that spawns a child process. Don't forget to check its exit code.

Copy link
Contributor

Choose a reason for hiding this comment

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

You need to pass -ea (to enable assertions) and -Dh2.forceAutoCommitOffOnCommit=true to the JVM and some argument to a program that can be used in the main(String[]) method to execute only the required method. In case of exception / error child process will be terminated with non-zero exit code and you can compare result of Process.waitFor() with zero to ensure that child process was terminated normally. TestMemoryUnmapper uses a more complicated logic with different exit codes that is not needed here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i'll go removing final modifier

Copy link
Contributor

@katzyn katzyn left a comment

Choose a reason for hiding this comment

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

Some minor issues.

h2/src/main/org/h2/api/ErrorCode.java Show resolved Hide resolved
h2/src/main/org/h2/api/ErrorCode.java Outdated Show resolved Hide resolved
h2/src/main/org/h2/api/ErrorCode.java Outdated Show resolved Hide resolved
h2/src/main/org/h2/jdbc/JdbcConnection.java Outdated Show resolved Hide resolved
h2/src/main/org/h2/jdbc/JdbcConnection.java Outdated Show resolved Hide resolved
@katzyn
Copy link
Contributor

katzyn commented Jun 19, 2019

Please, send a license statement as described here:
https://h2database.com/html/build.html#providing_patches

It's better to send it to our mailing list:
https://groups.google.com/forum/#!forum/h2-database
Mailing list is partially pre-moderated, some new messages don't appear immediately.

@nicoloboschi
Copy link
Contributor Author

@katzyn
Copy link
Contributor

katzyn commented Jun 19, 2019

Thanks!

@katzyn katzyn merged commit 34f761d into h2database:master Jun 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants