Implement scanner API for JDBC transactions #2702

brfrn169 · 2025-05-28T07:32:51Z

Description

This PR adds a scanner API implementation for JDBC transactions.

Note that we are working on this feature in the add-scanner-api-to-transaction-abstraction feature branch.

Related issues and/or PRs

Add getScanner() method to transaction interfaces #2698

Changes made

Added a scanner API implementation for JDBC transactions.

Checklist

The following is a best-effort checklist. If any items in this checklist are not applicable to this PR or are dependent on other, unmerged PRs, please still mark the checkboxes after you have read and understood each item.

I have commented my code, particularly in hard-to-understand areas.
I have updated the documentation to reflect the changes.
I have considered whether similar issues could occur in other products, components, or modules if this PR is for bug fixes.
Any remaining open issues linked to this PR are documented and up-to-date (Jira, GitHub, etc.).
Tests (unit, integration, etc.) have been added for the changes.
My changes generate no new warnings.
Any dependent changes in other PRs have been merged and published.

Additional notes (optional)

N/A

Release notes

N/A

Copilot

Pull Request Overview

This PR implements the scanner API for JDBC-based transactions by adding getScanner support in both JdbcTransaction and JdbcTransactionManager, extending ScannerImpl to optionally close its connection, updating JdbcService with a new overload, and adding comprehensive unit tests.

Added getScanner in JdbcTransaction and JdbcTransactionManager with proper commit/rollback semantics
Extended ScannerImpl to accept a closeConnectionOnClose flag and updated JdbcService to pass it
Introduced a new CoreError for scanner failures and added/updated unit tests for scanner behaviors

Reviewed Changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
core/src/main/java/com/scalar/db/transaction/jdbc/JdbcTransaction.java	Implemented `getScanner` with exception translation
core/src/main/java/com/scalar/db/transaction/jdbc/JdbcTransactionManager.java	Provided transactional wrapper via `AbstractTransactionManagerCrudOperableScanner`
core/src/main/java/com/scalar/db/storage/jdbc/ScannerImpl.java	Added `closeConnectionOnClose` flag to close connection on close
core/src/main/java/com/scalar/db/storage/jdbc/JdbcService.java	Added `getScanner` overload to pass connection-close flag
core/src/main/java/com/scalar/db/common/error/CoreError.java	Introduced `JDBC_TRANSACTION_GETTING_SCANNER_FAILED` error code
core/src/test/java/com/scalar/db/transaction/jdbc/JdbcTransactionTest.java	Added unit tests for `JdbcTransaction.getScanner`
core/src/test/java/com/scalar/db/transaction/jdbc/JdbcTransactionManagerTest.java	Added unit tests for `JdbcTransactionManager.getScanner`
core/src/test/java/com/scalar/db/storage/jdbc/JdbcDatabaseTest.java	Updated test to use new `ScannerImpl` constructor
core/src/integration-test/java/com/scalar/db/transaction/jdbc/JdbcTransactionIntegrationTest.java	Removed disabled integration tests

Comments suppressed due to low confidence (4)

core/src/main/java/com/scalar/db/common/error/CoreError.java:1185

[nitpick] The new error message "Getting the scanner failed. Details: %s" is missing a trailing period to match the style of other messages. Consider ending it with ...Details: %s.".

JDBC_TRANSACTION_GETTING_SCANNER_FAILED(

core/src/integration-test/java/com/scalar/db/transaction/jdbc/JdbcTransactionIntegrationTest.java:44

Integration tests for getScanner on committed records were removed. Re-enable or implement these scenarios (getScanner_ScanGivenForCommittedRecord and manager_getScanner_ScanGivenForCommittedRecord) to ensure end-to-end coverage.

@Disabled("Implement later")

core/src/test/java/com/scalar/db/transaction/jdbc/JdbcTransactionTest.java:4

[nitpick] The static import assertThatCode is not used in this test class. Remove unused imports (including ArgumentMatchers.any if unused) to keep imports clean.

import static org.assertj.core.api.Assertions.assertThatCode;

core/src/test/java/com/scalar/db/storage/jdbc/JdbcDatabaseTest.java:101

The test should verify that closing the scanner actually closes the connection when closeConnectionOnClose is true. Add a call to scanner.close() and then verify(connection).close() to assert the new behavior.

.thenReturn(
            new ScannerImpl(resultInterpreter, connection, preparedStatement, resultSet, true));

core/src/main/java/com/scalar/db/storage/jdbc/ScannerImpl.java

feeblefakie

LGTM! Thank you!

Torch3333

LGTM, thank you!

komamitsu · 2025-05-29T05:32:16Z

core/src/main/java/com/scalar/db/transaction/jdbc/JdbcTransactionManager.java

+        }
+
+        try {
+          transaction.commit();


If CrudException is thrown from one() or all(), only scanner is closed and closed is set to true. In this case, it seems calling this close() method won't close this transaction?

If CrudException is thrown from one() or all(), we call rollbackTransaction(transaction) to roll back the transaction. After that, even if a user call the close() method, the method does nothing. Is there any problem?

I see. Sounds good!

I confused this with com.scalar.db.transaction.jdbc.JdbcTransaction#getScanner...

komamitsu · 2025-05-29T05:32:49Z

core/src/main/java/com/scalar/db/transaction/jdbc/JdbcTransactionManager.java

+        try {
+          return scanner.all();
+        } catch (CrudException e) {
+          closed.set(true);


[minor] How about moving this after scanner.close() so that calling ~~closed()~~ close() method can close scanner even if calling scanner.close() throws an exception?

If scanner.close() throws an exception, we catch it:
https://github.com/scalar-labs/scalardb/pull/2702/files#diff-ca47f7038a18b284fd927596315c778f3c0dc5258d0ae101bb73c158c64e4917R224

Do you still think it’s necessary to move closed.set(true)?

I only thought in the current implementation there is no way to re-try to close scanner by calling close(), since the flag is set to true before scanner.close() finishes successfully. The concern was not based on actual usages, and probably never mind.

core/src/main/java/com/scalar/db/common/error/CoreError.java

core/src/main/java/com/scalar/db/storage/jdbc/JdbcService.java

komamitsu

LGTM! 👍

komamitsu · 2025-05-29T06:06:30Z

core/src/main/java/com/scalar/db/transaction/jdbc/JdbcTransactionManager.java

+        try {
+          return scanner.all();
+        } catch (CrudException e) {
+          closed.set(true);


I only thought in the current implementation there is no way to re-try to close scanner by calling close(), since the flag is set to true before scanner.close() finishes successfully. The concern was not based on actual usages, and probably never mind.

komamitsu · 2025-05-29T06:20:18Z

core/src/main/java/com/scalar/db/transaction/jdbc/JdbcTransactionManager.java

+        }
+
+        try {
+          transaction.commit();


I confused this with com.scalar.db.transaction.jdbc.JdbcTransaction#getScanner...

Implement scanner API for JDBC transaction

2189a55

brfrn169 requested a review from Copilot May 28, 2025 07:32

brfrn169 self-assigned this May 28, 2025

Copilot AI reviewed May 28, 2025

View reviewed changes

core/src/main/java/com/scalar/db/storage/jdbc/ScannerImpl.java Show resolved Hide resolved

brfrn169 requested review from Torch3333, feeblefakie and komamitsu May 28, 2025 08:27

feeblefakie approved these changes May 28, 2025

View reviewed changes

Torch3333 approved these changes May 29, 2025

View reviewed changes

komamitsu reviewed May 29, 2025

View reviewed changes

brfrn169 requested review from Copilot and komamitsu and removed request for Copilot May 29, 2025 05:50

Copilot AI reviewed May 29, 2025

View reviewed changes

core/src/main/java/com/scalar/db/common/error/CoreError.java Show resolved Hide resolved

core/src/main/java/com/scalar/db/storage/jdbc/JdbcService.java Show resolved Hide resolved

komamitsu approved these changes May 29, 2025

View reviewed changes

brfrn169 merged commit 5042c49 into add-scanner-api-to-transaction-abstraction May 29, 2025
56 checks passed

brfrn169 deleted the implement-scanner-api-for-jdbc branch May 29, 2025 06:23

This was referenced Jun 2, 2025

Add more integration tests for scanner API #2724

Merged

Add scanner API to transaction abstraction #2729

Merged

Uh oh!

Implement scanner API for JDBC transactions #2702

Implement scanner API for JDBC transactions #2702

Uh oh!

Conversation

brfrn169 commented May 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related issues and/or PRs

Changes made

Checklist

Additional notes (optional)

Release notes

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

feeblefakie left a comment

Choose a reason for hiding this comment

Uh oh!

Torch3333 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

komamitsu May 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

komamitsu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

brfrn169 commented May 28, 2025 •

edited

Loading

komamitsu May 29, 2025 •

edited

Loading