feat: FIR-50249 support transactions in Core#643
feat: FIR-50249 support transactions in Core#643bogdantruta-firebolt wants to merge 2 commits intomasterfrom
Conversation
| * @param fireboltStatementService statement service to inject | ||
| * @throws SQLException if connection fails | ||
| */ | ||
| @TestOnly |
There was a problem hiding this comment.
I've seen @VisibleForTesting before but not this one. Does it do the same thing? If so let't keep the @VisibleForTesting instead. From what I know the sonar will fail if it sees actual prod code that tries to use a method annotated with @VisibleForTesting. I don't know about this annotation.
There was a problem hiding this comment.
That annotation is from a google package not present in this project. I was suggested this annotation by Intellij, but I thought that this only marks the constructor as being for tests for us, not for the compiler/Sonar. Do you think I should add the google package with @VisibleForTesting?
src/test/java/com/firebolt/jdbc/connection/FireboltConnectionServiceSecretTest.java
Outdated
Show resolved
Hide resolved
ptiurin
left a comment
ptiurin
left a comment
There was a problem hiding this comment.
apart from this looks good
| public void ensureTransactionForQueryExecution() throws SQLException { | ||
| validateConnectionIsNotClose(); | ||
| if (sessionProperties.getTransactionId() == null) { | ||
| inTransaction = false; | ||
| } | ||
|
|
||
| if (executingTransactionCommand || autoCommit) { | ||
| return; | ||
| } | ||
|
|
||
| if (!inTransaction) { | ||
| executingTransactionCommand = true; | ||
| try (Statement statement = createStatement()) { | ||
| statement.execute("BEGIN TRANSACTION"); | ||
| inTransaction = true; |
There was a problem hiding this comment.
Outside of the scope of this PR, but is this thread-safe? Can there be a case where connection is shared across threads and one of them starts a transaction, but we check before the transaction id is updated and so set the inTransaction to false?
There was a problem hiding this comment.
Good one, didn't think of it. To be safe I can move the rollback inside the synchronized block, but I don't think this should happen since only one thread should run the close and it should be after running all queries. Plus I don't think JDBC is assumed to be used with more than one thread and just one connection
|
3 participants
Added support for transactions for Firebolt Core.
Firebolt Core does not support multiple transactions, that's why some integration tests are not being run on Core, but it supports one transaction at a time, with commit and rollback. Also it seems to support running queries outside of active transactions.
Also changed the default behavior on close to rollback any pending transactions so Core will not remain in a broken state due to pending transactions.