Add READ_COMMITTED isolation #2803

brfrn169 · 2025-06-20T02:26:13Z

Description

This PR adds support for the READ_COMMITTED isolation level.

The behavior specific to READ_COMMITTED is as follows:

When reading an uncommitted record with a status of PREPARED or DELETED, always return a record created from the before image without the coordinator check.
In read-only mode, lazy recovery is not performed.

Related issues and/or PRs

N/A

Changes made

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

Added the READ_COMMITTED isolation level, which offers better performance, especially for low-contention workloads.

Copilot

Pull Request Overview

This PR adds READ_COMMITTED isolation support to the consensus commit transaction module while refactoring configuration usage. Key changes include:

Adding READ_COMMITTED to the Isolation enum and modifying related logic in recovery and CRUD handling.
Refactoring ConsensusCommitManager and test classes to use the new isolation configuration.
Updating exception handling in commit and preparation flows to correctly propagate conflict errors.

Reviewed Changes

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

Show a summary per file

File	Description
integration-test/src/main/java/com/scalar/db/transaction/consensuscommit/ConsensusCommitNullMetadataIntegrationTestBase.java	Removed the consensusCommitConfig parameter and now passing the isolation value directly.
core/src/test/java/com/scalar/db/transaction/consensuscommit/TwoPhaseConsensusCommitTest.java	Added tests for the new exception mapping (e.g. PreparationConflictException).
core/src/test/java/com/scalar/db/transaction/consensuscommit/RecoveryExecutorTest.java	Updated tests to validate the new recovery types for different isolation levels.
core/src/test/java/com/scalar/db/transaction/consensuscommit/ConsensusCommitManagerTest.java	Adjusted initialization to use local config and store the isolation value.
core/src/main/java/com/scalar/db/transaction/consensuscommit/Isolation.java	Introduced READ_COMMITTED as a new supported isolation level.
core/src/main/java/com/scalar/db/transaction/consensuscommit/CrudHandler.java	Updated recovery execution to select recovery type based on the snapshot isolation value.
core/src/main/java/com/scalar/db/transaction/consensuscommit/ConsensusCommitManager.java	Refactored config usage to pass isolation to dependent objects and updated begin methods accordingly.
core/src/main/java/com/scalar/db/transaction/consensuscommit/ConsensusCommit.java	Updated commit() to catch CrudConflictException and throw CommitConflictException.

Comments suppressed due to low confidence (1)

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

[nitpick] The catch block for ExecutionException rethrows a CrudConflictException when the cause is a conflict. Consider adding a brief comment explaining why conflicts are handled separately to improve code clarity.

        if (e.getCause() instanceof CrudConflictException) {

Copilot · 2025-06-20T02:26:47Z

...om/scalar/db/transaction/consensuscommit/ConsensusCommitNullMetadataIntegrationTestBase.java

            parallelExecutor,
            recoveryExecutor,
            commit,
+            consensusCommitConfig.getIsolation(),


The test still calls consensusCommitConfig.getIsolation() even though consensusCommitConfig was removed as a constructor argument. Update the test initialization to retrieve the isolation value from the appropriate source to match the refactored constructor.

Suggested change

consensusCommitConfig.getIsolation(),

databaseConfig.getIsolation(),

brfrn169 · 2025-06-20T02:41:15Z

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

+  public enum RecoveryType {
+    RETURN_LATEST_RESULT_AND_RECOVER,
+    RETURN_COMMITTED_RESULT_AND_RECOVER,
+    RETURN_COMMITTED_RESULT_AND_NOT_RECOVER
+  }


Introduce RecoveryType in RecoveryExecutor.

RETURN_LATEST_RESULT_AND_RECOVER: This is the previous behavior. Return the latest record with the coordinator check, then start lazy recovery.
RETURN_COMMITTED_RESULT_AND_RECOVER: Return a record created from the before image without the coordinator check, then start lazy recovery.
RETURN_COMMITTED_RESULT_AND_NOT_RECOVER: Return a record created from the before image without the coordinator check. This type does not start lazy recovery.

brfrn169 · 2025-06-20T02:44:34Z

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

+    if (snapshot.getIsolation() == Isolation.READ_COMMITTED) {
+      // In READ_COMMITTED isolation
+
+      if (readOnly) {
+        // In read-only mode, we don't recover the record, but return the committed result
+        recoveryType = RecoveryExecutor.RecoveryType.RETURN_COMMITTED_RESULT_AND_NOT_RECOVER;
+      } else {
+        // In read-write mode, we recover the record and return the committed result
+        recoveryType = RecoveryExecutor.RecoveryType.RETURN_COMMITTED_RESULT_AND_RECOVER;
+      }
+    } else {
+      // In SNAPSHOT or SERIALIZABLE isolation, we always recover the record and return the latest
+      // result
+      recoveryType = RecoveryExecutor.RecoveryType.RETURN_LATEST_RESULT_AND_RECOVER;
+    }


The RecoveryType is chosen based in the isolation level and whether the transaction is read-only or not.

brfrn169 · 2025-06-20T02:49:01Z

...va/com/scalar/db/transaction/consensuscommit/ConsensusCommitSpecificIntegrationTestBase.java

Added many additional integration tests. Please refer to them if you want to know the details about the behavioral differences between isolation levels and read-only mode, etc.

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

komamitsu

LGTM! 👍

feeblefakie

LGTM! Thank you!

Torch3333

LGTM, thank you!

Add READ_COMMITTED isolation

ca4adf2

brfrn169 self-assigned this Jun 20, 2025

brfrn169 added the enhancement New feature or request label Jun 20, 2025

brfrn169 requested a review from Copilot June 20, 2025 02:26

Copilot AI reviewed Jun 20, 2025

View reviewed changes

brfrn169 commented Jun 20, 2025

View reviewed changes

brfrn169 requested review from Torch3333, feeblefakie and komamitsu June 20, 2025 02:49

komamitsu reviewed Jun 20, 2025

View reviewed changes

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

komamitsu approved these changes Jun 20, 2025

View reviewed changes

feeblefakie approved these changes Jun 20, 2025

View reviewed changes

Torch3333 approved these changes Jun 20, 2025

View reviewed changes

Merge branch 'master' into add-read-committed-isolation

18e759c

brfrn169 merged commit daf4d88 into master Jun 20, 2025
106 of 108 checks passed

brfrn169 deleted the add-read-committed-isolation branch June 20, 2025 06:27

feeblefakie mentioned this pull request Jun 20, 2025

Backport to branch(3) : Add READ_COMMITTED isolation #2806

Merged

brfrn169 added a commit that referenced this pull request Jun 20, 2025

Add READ_COMMITTED isolation (#2803)

04c6665

brfrn169 added a commit that referenced this pull request Jun 20, 2025

Add READ_COMMITTED isolation (#2803)

137b672

brfrn169 mentioned this pull request Jun 24, 2025

Support READ-COMMITTED isolation scalar-labs/scalar-jepsen#147

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Add READ_COMMITTED isolation #2803

Add READ_COMMITTED isolation #2803

Uh oh!

brfrn169 commented Jun 20, 2025 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Jun 20, 2025

Uh oh!

brfrn169 Jun 20, 2025

Uh oh!

brfrn169 Jun 20, 2025

Uh oh!

brfrn169 Jun 20, 2025 •

edited

Loading

Uh oh!

Uh oh!

komamitsu left a comment

Uh oh!

feeblefakie left a comment

Uh oh!

Torch3333 left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

	consensusCommitConfig.getIsolation(),
	databaseConfig.getIsolation(),

Add READ_COMMITTED isolation #2803

Add READ_COMMITTED isolation #2803

Uh oh!

Conversation

brfrn169 commented Jun 20, 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!

Copilot AI Jun 20, 2025

Choose a reason for hiding this comment

Uh oh!

brfrn169 Jun 20, 2025

Choose a reason for hiding this comment

Uh oh!

brfrn169 Jun 20, 2025

Choose a reason for hiding this comment

Uh oh!

brfrn169 Jun 20, 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!

Uh oh!

komamitsu left a comment

Choose a reason for hiding this comment

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!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

brfrn169 commented Jun 20, 2025 •

edited

Loading

brfrn169 Jun 20, 2025 •

edited

Loading