Implement scanner API for Consensus Commit #2711

brfrn169 · 2025-05-29T06:27:16Z

Description

This PR adds a scanner API implementation for Consensus Commit 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 Consensus Commit 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 a scanner API for Consensus Commit transactions by updating multiple transaction and snapshot related classes and enhancing the corresponding integration and unit tests. Key changes include:

Implementation of scanner methods (getScanner, one, all, close) in transaction manager classes.
Refactoring of snapshot and CRUD handler logic to incorporate scanner state and error recovery.
Extensive updates to both integration and core tests to validate the new scanner API behavior.

Reviewed Changes

Copilot reviewed 16 out of 16 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
integration-test/src/main/java/com/scalar/db/transaction/consensuscommit/TwoPhaseConsensusCommitIntegrationTestBase.java	Removed disabled tests to focus on active scanner API tests.
core/src/main/java/com/scalar/db/transaction/consensuscommit/TwoPhaseConsensusCommitManager.java	Replaced unsupported scanner method with a concrete implementation that wraps scanner behavior and ensures transaction rollback on errors.
core/src/main/java/com/scalar/db/transaction/consensuscommit/CrudHandler.java	Added scanner management (tracking, closing) and new methods getScanner()/closeScanners() for proper lifecycle management.
core/src/main/java/com/scalar/db/transaction/consensuscommit/Snapshot.java	Extended snapshot handling to include scannerSet and adjusted validation logic for not fully scanned scanners.
core/src/main/java/com/scalar/db/transaction/consensuscommit/ConsensusCommit*.java	Updated scanner implementations to use new recovery and lazy evaluation strategies.
core/src/main/java/com/scalar/db/common/error/CoreError.java	Added new error codes for scanner closure requirements.

Comments suppressed due to low confidence (1)

core/src/main/java/com/scalar/db/transaction/consensuscommit/TwoPhaseConsensusCommitManager.java:193

[nitpick] Consider adding a brief inline explanation describing the error recovery flow—specifically how rollbackTransaction is invoked when scanner acquisition fails—to improve the readability and maintainability of the scanner API implementation.

public Scanner getScanner(Scan scan) throws CrudException {

Copilot · 2025-05-29T06:27:48Z

core/src/main/java/com/scalar/db/transaction/consensuscommit/CrudHandler.java

+    return scanners.stream().allMatch(ConsensusCommitScanner::isClosed);
+  }
+
+  public void closeScanners() throws CrudException {


Consider clearing the internal 'scanners' list at the end of closeScanners() to release references to closed scanners and to avoid potential side effects on subsequent scanner operations.

Copilot · 2025-05-29T06:27:48Z

core/src/main/java/com/scalar/db/transaction/consensuscommit/Snapshot.java

@@ -604,6 +624,11 @@ private void validateScanResults(
        return;
      }

+      if (notFullyScannedScanner) {


[nitpick] Consider adding an inline comment explaining why further validation is skipped for not fully scanned scanners, to clarify the intentional trade-off in snapshot consistency checks.

brfrn169 · 2025-05-29T06:32:00Z

core/src/main/java/com/scalar/db/transaction/consensuscommit/ConsensusCommit.java

@@ -213,6 +249,10 @@ public void mutate(List<? extends Mutation> mutations) throws CrudException {

  @Override
  public void commit() throws CommitException, UnknownTransactionStatusException {
+    if (!crud.areAllScannersClosed()) {


Check if all the scanners are closed before committing a transaction.

brfrn169 · 2025-05-29T06:36:00Z

core/src/main/java/com/scalar/db/transaction/consensuscommit/CrudHandler.java

+    Optional<LinkedHashMap<Snapshot.Key, TransactionResult>> resultsInSnapshot =
+        snapshot.getResults(scan);
+    if (resultsInSnapshot.isPresent()) {
+      scanner =
+          new ConsensusCommitSnapshotScanner(scan, originalProjections, resultsInSnapshot.get());
+    } else {
+      scanner = new ConsensusCommitStorageScanner(scan, originalProjections);
+    }


If the scan result of the same scan is in the scanSet in the snapshot, we use it. If not, we scan the storage directly.

brfrn169 · 2025-05-29T06:36:27Z

core/src/main/java/com/scalar/db/transaction/consensuscommit/CrudHandler.java

+  }
+
+  @NotThreadSafe
+  private class ConsensusCommitStorageScanner extends AbstractTransactionCrudOperableScanner


The scanner implementation for scanning the storage.

brfrn169 · 2025-05-29T06:36:50Z

core/src/main/java/com/scalar/db/transaction/consensuscommit/CrudHandler.java

+  }
+
+  @NotThreadSafe
+  private class ConsensusCommitSnapshotScanner extends AbstractTransactionCrudOperableScanner


The scanner implementation for scanning the result in the snapshot.

brfrn169 · 2025-05-29T06:45:16Z

core/src/main/java/com/scalar/db/transaction/consensuscommit/CrudHandler.java

+      if (fullyScanned) {
+        // If the scanner is fully scanned, we can treat it as a normal scan, and put the results
+        // into the scan set
+        snapshot.putIntoScanSet(scan, results);
+      } else {
+        // If the scanner is not fully scanned, put the results into the scanner set
+        snapshot.putIntoScannerSet(scan, results);
+      }


This is a tricky part. The behavior depends on whether the storage scanner has been fully scanned.

If the storage scanner has been fully scanned, we can treat it as a normal scan and put the result into the scan set, just like a regular scan.

If the storage scanner has not been fully scanned, we put the results into the scanner set.

The reason why we need to differently handle the not-fully-scanned scan is that there can be remaining records? If so, how about reading the remaining records in this method and adding them to results so that we can treat the scan as a fully scanned one?

The reason why we need to differently handle the not-fully-scanned scan is that there can be remaining records?

Yes, because of that, the not-fully-scanned scan should need special handling in the serializable validation, similar to scan with limit.

If so, how about reading the remaining records in this method and adding them to results so that we can treat the scan as a fully scanned one?

In that case, if the remaining records are large, we would consume a lot of memory. One of the advantages of the scanner API is that it only consumes memory as records are scanned. So, we should avoid doing that.

I see. That sounds reasonable.

brfrn169 · 2025-05-29T06:46:43Z

core/src/main/java/com/scalar/db/transaction/consensuscommit/Snapshot.java

@@ -64,6 +64,9 @@ public class Snapshot {
  private final Map<Key, Put> writeSet;
  private final Map<Key, Delete> deleteSet;

+  // The scanner set used to store information about scanners that are not fully scanned
+  private final List<ScannerInfo> scannerSet;


Added scannerSet that's used to store information about scanners that are not fully scanned.

brfrn169 · 2025-05-29T06:47:11Z

core/src/main/java/com/scalar/db/transaction/consensuscommit/Snapshot.java

+    // Scanner set is re-validated to check if there is no anti-dependency
+    for (ScannerInfo scannerInfo : scannerSet) {
+      tasks.add(() -> validateScanResults(storage, scannerInfo.scan, scannerInfo.results, true));
    }


Added serializable validation for the scanner set.

brfrn169 · 2025-05-29T06:53:53Z

core/src/main/java/com/scalar/db/transaction/consensuscommit/Snapshot.java

+      if (notFullyScannedScanner) {
+        // If the scanner is not fully scanned, no further checks are needed
+        return;
+      }


If the scanner has not been fully scanned, no further check is needed. This is similar to the case when a limit is specified:

scalardb/core/src/main/java/com/scalar/db/transaction/consensuscommit/Snapshot.java

Lines 622 to 625 in 1717cfb

if (scan.getLimit() != 0 && results.size() == scan.getLimit()) {

// We’ve already checked up to the limit, so no further checks are needed

return;

}

komamitsu · 2025-05-30T02:17:44Z

core/src/main/java/com/scalar/db/transaction/consensuscommit/ConsensusCommit.java

+          return scanner.one();
+        } catch (UncommittedRecordException e) {
+          lazyRecovery(e);
+          throw e;


[minor] Is there any reason not to close scanner here while ConsensusCommitManager.getScanner() closes that?

We don't close the scanner here, but we close here:
https://github.com/scalar-labs/scalardb/pull/2711/files#diff-acc93f1366b03168017001646c7217f7dd1f3c7cbe8a41b6a9a8484dc5ef0905R406

The reason is probably ConsensusCommitManager commits after one scan, whereas ConsensusCommit commits after doing several operations?

(Oops, sorry I thought here was in catch (CrudException e) { ... } block).

I just thought we can immediately close the failed scanner as well as ConsensusCommitManager.getScanner(), for consistency and a bit faster resource release. It's just out of curiosity.

Ah, sorry for the lack of explanation.

This anonymous Scanner class wraps the scanner instance returned by CrudHandler here:
https://github.com/scalar-labs/scalardb/pull/2711/files#diff-ec7fb790c921b21b0690f23d557c721585b0c83ef32b729e7dada7009d6142d0R104

The CrudHandler.getScanner() method returns an instance of ConsensusCommitStorageScanner if the scan result is not present in the snapshot.

The ConsensusCommitStorageScanner.one() method immediately closes the underlying scanner when a failure occurs:
https://github.com/scalar-labs/scalardb/pull/2711/files#diff-acc93f1366b03168017001646c7217f7dd1f3c7cbe8a41b6a9a8484dc5ef0905R424-R452

So, the behavior is consistent with ConsensusCommitManager.getScanner().

By the way, the anonymous Scanner class simply executes lazy recovery when an UncommittedRecordException is caught.

Thanks for the explanation! Sounds good 💯

komamitsu · 2025-05-30T03:04:32Z

core/src/main/java/com/scalar/db/transaction/consensuscommit/CrudHandler.java

+      if (fullyScanned) {
+        // If the scanner is fully scanned, we can treat it as a normal scan, and put the results
+        // into the scan set
+        snapshot.putIntoScanSet(scan, results);
+      } else {
+        // If the scanner is not fully scanned, put the results into the scanner set
+        snapshot.putIntoScannerSet(scan, results);
+      }


The reason why we need to differently handle the not-fully-scanned scan is that there can be remaining records? If so, how about reading the remaining records in this method and adding them to results so that we can treat the scan as a fully scanned one?

feeblefakie

Overall, looking good!
I will take another look.

feeblefakie · 2025-05-30T03:55:46Z

core/src/main/java/com/scalar/db/transaction/consensuscommit/CrudHandler.java

+
+    @Override
+    public List<Result> all() throws CrudException {
+      List<Result> results = new ArrayList<>();


Can we return the results in the class variable by doing ImmutabeList.copyOf(results) or something instead of creating another list?

You’re referring to the following class variable results, right?
https://github.com/scalar-labs/scalardb/pull/2711/files#diff-acc93f1366b03168017001646c7217f7dd1f3c7cbe8a41b6a9a8484dc5ef0905R413

If so, the class variable results contains all the fetched results, but the all() method is supposed to return only the remaining results.

For example, suppose the scan result is [1, 2, 3, 4, 5]. In the following case, the all() method should return [3, 4, 5]:

Result result1 = scanner.one(); // returns 1 Result result2 = scanner.one(); // returns 2 List<Result> results = scanner.all(); // returns [3, 4, 5]

That’s why we can’t simply return something like ImmutableList.copyOf(this.results) in the all() method. Thanks!

Oh, OK, I misunderstood the behavior.
I don't think we can change it for backward compatibility, but the method name might be confusing since all() sounds like returning all results, not remaining. (it's probably my mistake though)

feeblefakie · 2025-05-30T10:48:06Z

core/src/main/java/com/scalar/db/transaction/consensuscommit/ConsensusCommit.java

+          return scanner.one();
+        } catch (UncommittedRecordException e) {
+          lazyRecovery(e);
+          throw e;


The reason is probably ConsensusCommitManager commits after one scan, whereas ConsensusCommit commits after doing several operations?

komamitsu

LGTM! 👍

komamitsu · 2025-05-30T11:32:54Z

core/src/main/java/com/scalar/db/transaction/consensuscommit/ConsensusCommit.java

+          return scanner.one();
+        } catch (UncommittedRecordException e) {
+          lazyRecovery(e);
+          throw e;


(Oops, sorry I thought here was in catch (CrudException e) { ... } block).

I just thought we can immediately close the failed scanner as well as ConsensusCommitManager.getScanner(), for consistency and a bit faster resource release. It's just out of curiosity.

komamitsu · 2025-05-30T11:39:00Z

core/src/main/java/com/scalar/db/transaction/consensuscommit/CrudHandler.java

+      if (fullyScanned) {
+        // If the scanner is fully scanned, we can treat it as a normal scan, and put the results
+        // into the scan set
+        snapshot.putIntoScanSet(scan, results);
+      } else {
+        // If the scanner is not fully scanned, put the results into the scanner set
+        snapshot.putIntoScannerSet(scan, results);
+      }


I see. That sounds reasonable.

Torch3333

I left a minor comment, PTAL.
Besides this, LGTM, thank you.

core/src/main/java/com/scalar/db/transaction/consensuscommit/CrudHandler.java

…rudHandler.java Co-authored-by: Vincent Guilpain <[email protected]>

feeblefakie

LGTM! Thank you!

feeblefakie · 2025-06-02T08:42:07Z

core/src/main/java/com/scalar/db/transaction/consensuscommit/CrudHandler.java

+
+    @Override
+    public List<Result> all() throws CrudException {
+      List<Result> results = new ArrayList<>();


Oh, OK, I misunderstood the behavior.
I don't think we can change it for backward compatibility, but the method name might be confusing since all() sounds like returning all results, not remaining. (it's probably my mistake though)

brfrn169 requested a review from Copilot May 29, 2025 06:27

brfrn169 self-assigned this May 29, 2025

Copilot AI reviewed May 29, 2025

View reviewed changes

Implement scanner API for Consensus Commit

bdb959c

brfrn169 force-pushed the implement-scanner-for-consensus-commit branch from 6e3bde3 to bdb959c Compare May 29, 2025 06:28

[skip ci] revise a comment

1717cfb

brfrn169 commented May 29, 2025

View reviewed changes

brfrn169 requested review from komamitsu, feeblefakie and Torch3333 May 29, 2025 06:56

komamitsu reviewed May 30, 2025

View reviewed changes

Fix

34525dd

brfrn169 requested a review from komamitsu May 30, 2025 06:06

feeblefakie reviewed May 30, 2025

View reviewed changes

komamitsu approved these changes May 30, 2025

View reviewed changes

brfrn169 requested a review from feeblefakie May 31, 2025 15:09

Torch3333 approved these changes Jun 2, 2025

View reviewed changes

core/src/main/java/com/scalar/db/transaction/consensuscommit/CrudHandler.java Outdated Show resolved Hide resolved

Update core/src/main/java/com/scalar/db/transaction/consensuscommit/C…

aad0ad2

…rudHandler.java Co-authored-by: Vincent Guilpain <[email protected]>

feeblefakie approved these changes Jun 2, 2025

View reviewed changes

feeblefakie merged commit 7b91326 into add-scanner-api-to-transaction-abstraction Jun 2, 2025
55 checks passed

brfrn169 deleted the implement-scanner-for-consensus-commit branch June 2, 2025 10:34

This was referenced Jun 2, 2025

Add more integration tests for scanner API #2724

Merged

Add scanner API to transaction abstraction #2729

Merged

	if (scan.getLimit() != 0 && results.size() == scan.getLimit()) {
	// We’ve already checked up to the limit, so no further checks are needed
	return;
	}

Implement scanner API for Consensus Commit #2711

Implement scanner API for Consensus Commit #2711

Uh oh!

Conversation

brfrn169 commented May 29, 2025

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!

Copilot AI May 29, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI May 29, 2025

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!

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!

brfrn169 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!

brfrn169 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!

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!

feeblefakie left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brfrn169 May 30, 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!

komamitsu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

brfrn169 May 29, 2025 •

edited

Loading

brfrn169 May 29, 2025 •

edited

Loading

brfrn169 May 30, 2025 •

edited

Loading