Add further optimizations for Consensus Commit #2764

brfrn169 · 2025-06-13T06:37:32Z

Description

This PR adds further optimizations for Consensus Commit.

It introduces a single-operation mode, a mode for transactions executed without explicitly beginning a transaction, such as DistributedTransactionManager.get(), DistributedTransactionManager.scan(), and DistributedTransactionManager.insert().

Details of the optimizations are provided in the inline comments.

Related issues and/or PRs

N/A

Changes made

Added some inline comments. Please take a look for the details.

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 introduces additional optimizations for Consensus Commit by updating transaction APIs and tests to use a more flexible begin method with extra flags (readOnly and singleOperation). Key changes include:

Refactoring internal transaction creation to use a new begin(txId, isolation, readOnly, singleOperation) signature.
Updating numerous test classes to verify the new multi-argument begin calls.
Enhancing snapshot read logic and CRUD handler behavior with single-operation mode optimizations.

Reviewed Changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
integration-test/src/main/java/com/scalar/db/transaction/singlecrudoperation/SingleCrudOperationTransactionIntegrationTestBase.java	Added a disabled test for multiple mutations in single CRUD operations.
integration-test/src/main/java/com/scalar/db/transaction/consensuscommit/ConsensusCommitSpecificIntegrationTestBase.java	Introduced various new tests using the updated begin method with additional parameters.
core/src/test/java/...	Updated tests to verify the new begin(anyString(), eq(Isolation.SNAPSHOT), …) signatures.
core/src/main/java/com/scalar/db/transaction/consensuscommit/TwoPhaseConsensusCommitManager.java	Refactored begin methods with extra flags and updated visibility of internal methods.
core/src/main/java/com/scalar/db/transaction/consensuscommit/Snapshot.java	Changed mergeResult method access from private to public.
core/src/main/java/com/scalar/db/transaction/consensuscommit/CrudHandler.java	Added a singleOperation flag and helper methods for conditional snapshot reading.
core/src/main/java/com/scalar/db/transaction/consensuscommit/ConsensusCommitManager.java	Updated transaction begin methods and introduced a beginSingleOperation helper.

Comments suppressed due to low confidence (4)

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

[nitpick] A 'singleOperation' flag has been introduced to control snapshot read behavior; consider adding or updating documentation to clearly explain its purpose and effects on CRUD operations.

private final boolean singleOperation;

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

[nitpick] Changing the visibility of mergeResult from private to public alters encapsulation; ensure that this exposure is intentional and is well documented for API consumers.

public Optional<TransactionResult> mergeResult(Key key, Optional<TransactionResult> result)

core/src/main/java/com/scalar/db/transaction/consensuscommit/ConsensusCommitManager.java:254

The new beginSingleOperation method is used to optimize single-operation reads; verify that the corresponding tests fully cover scenarios where snapshot reads are bypassed as intended.

private DistributedTransaction beginSingleOperation(boolean readOnly) {

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

Changing the visibility of this begin method to private may affect testability—ensure that tests relying on internal behavior are updated accordingly.

private TwoPhaseCommitTransaction begin(String txId, Isolation isolation) {

brfrn169 · 2025-06-13T06:44:35Z

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

+  // Whether the transaction is in single-operation mode or not. Single-operation mode refers to
+  // executing a CRUD operation directly through `DistributedTransactionManager` without explicitly
+  // beginning a transaction.
+  private final boolean singleOperation;


Introduced single-operation mode, a mode for transactions executed without explicitly beginning a transaction, such as DistributedTransactionManager.get(), DistributedTransactionManager.scan(), and DistributedTransactionManager.insert().

brfrn169 · 2025-06-13T06:56:14Z

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

+  private boolean isSnapshotReadRequired() {
+    // In single-operation mode, we don't need snapshot read
+    return !singleOperation;
+  }
+
+  private boolean isValidationOrSnapshotReadRequired() {
+    return snapshot.isValidationRequired() || isSnapshotReadRequired();
+  }
+
+  private void putIntoGetSetInSnapshot(Get get, Optional<TransactionResult> result) {
+    // If neither validation nor snapshot read is required, we don't need to put the result into
+    // the get set
+    if (isValidationOrSnapshotReadRequired()) {
+      snapshot.putIntoGetSet(get, result);
+    }
+  }
+
+  private void putIntoScanSetInSnapshot(
+      Scan scan, LinkedHashMap<Snapshot.Key, TransactionResult> results) {
+    // If neither validation nor snapshot read is required, we don't need to put the results into
+    // the scan set
+    if (isValidationOrSnapshotReadRequired()) {
+      snapshot.putIntoScanSet(scan, results);
+    }
+  }
+
+  private void putIntoScannerSetInSnapshot(
+      Scan scan, LinkedHashMap<Snapshot.Key, TransactionResult> results) {
+    // if validation is not required, we don't need to put the results into the scanner set
+    if (snapshot.isValidationRequired()) {
+      snapshot.putIntoScannerSet(scan, results);
+    }
+  }
+
  private void verifyNoOverlap(Scan scan, Map<Snapshot.Key, TransactionResult> results) {
-    // In read-only mode, we don't need to verify the overlap
-    if (!readOnly) {
+    // In either read-only mode or single-operation mode, we don't need to verify the overlap
+    if (!readOnly && !singleOperation) {
      snapshot.verifyNoOverlap(scan, results);
    }
  }


Here is the main part of the optimizations.

The optimizations can be summarized as follows:

If snapshot reads and serializable validation are not required, we can avoid adding records to the getSet and scanSet, thereby reducing memory usage. This is because they are only used for snapshot reads and serializable validation.

If serializable validation is not required, we can skip adding ScannerInfo to the scannerSet, since it is used solely for that purpose.

In addition to read-only mode, we can also skip verifying overlaps between the scan and the records in the writeSet and deleteSet in single-operation mode.

If a single-crud operation is a self-join reading the same record more than twice, is it OK not to use snapshot read?

The single-operation mode here means the mode for transactions that are executed without explicitly beginning a transaction, such as the following methods:

DistributedTransactionManger transactionManager = ...; Optional<Result> result = transactionManager.get(Get.newBuilder()...); List<Result> results = transactionManager.scan(Scan.newBuilder()...); Scanner scanner = transactionManager.getScanner(Scan.newBuilder()...); transactionManager.insert(Insert.newBuilder()...); transactionManager.upsert(Upsert.newBuilder()...); transactionManager.update(Update.newBuilder()...); transactionManager.delete(Delete.newBuilder()...); transactionManager.mutate(Arrays.asList(Delete.newBuilder()..., Update.newBuilder()...));

In these cases, the transaction is committed immediately after the operation is done. So we don't need snapshot reads in such cases. Does this answer your question?

@brfrn169 Probably, I am confused with single-crud operation transaction and single-operation mode.
If they are different things, it's pretty confusing so I think we should change the name.

Ah, okay. This isn’t related to single CRUD operation transactions — it’s specific to Consensus Commit.

Do you have any suggestions for what we should call transactions that are executed without explicitly beginning a transaction?

komamitsu

@brfrn169 We don't need to confirm the number of operations in a transaction is only 1 at the commit phase, if the single operation feature is enabled on the transaction?

komamitsu · 2025-06-16T00:59:55Z

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

    private final Scanner scanner;

-    private final LinkedHashMap<Snapshot.Key, TransactionResult> results = new LinkedHashMap<>();
+    private final LinkedHashMap<Snapshot.Key, TransactionResult> results;


This should have @Nullable?

Fixed in 61b2860. Thanks!

feeblefakie

Left some comments and questions. PTAL!

feeblefakie · 2025-06-16T04:13:02Z

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

+            .map(
+                r ->
+                    new FilteredResult(r, originalProjections, metadata, isIncludeMetadataEnabled));
+      } else {


How about handling null/empty conjunction in the mergeResult method to unify the operations in the if/else clauses?

Fixed in cefe83b. Thanks!

feeblefakie · 2025-06-16T04:19:23Z

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

+  private boolean isSnapshotReadRequired() {
+    // In single-operation mode, we don't need snapshot read
+    return !singleOperation;
+  }
+
+  private boolean isValidationOrSnapshotReadRequired() {
+    return snapshot.isValidationRequired() || isSnapshotReadRequired();
+  }
+
+  private void putIntoGetSetInSnapshot(Get get, Optional<TransactionResult> result) {
+    // If neither validation nor snapshot read is required, we don't need to put the result into
+    // the get set
+    if (isValidationOrSnapshotReadRequired()) {
+      snapshot.putIntoGetSet(get, result);
+    }
+  }
+
+  private void putIntoScanSetInSnapshot(
+      Scan scan, LinkedHashMap<Snapshot.Key, TransactionResult> results) {
+    // If neither validation nor snapshot read is required, we don't need to put the results into
+    // the scan set
+    if (isValidationOrSnapshotReadRequired()) {
+      snapshot.putIntoScanSet(scan, results);
+    }
+  }
+
+  private void putIntoScannerSetInSnapshot(
+      Scan scan, LinkedHashMap<Snapshot.Key, TransactionResult> results) {
+    // if validation is not required, we don't need to put the results into the scanner set
+    if (snapshot.isValidationRequired()) {
+      snapshot.putIntoScannerSet(scan, results);
+    }
+  }
+
  private void verifyNoOverlap(Scan scan, Map<Snapshot.Key, TransactionResult> results) {
-    // In read-only mode, we don't need to verify the overlap
-    if (!readOnly) {
+    // In either read-only mode or single-operation mode, we don't need to verify the overlap
+    if (!readOnly && !singleOperation) {
      snapshot.verifyNoOverlap(scan, results);
    }
  }


If a single-crud operation is a self-join reading the same record more than twice, is it OK not to use snapshot read?

brfrn169 · 2025-06-16T04:39:08Z

We don't need to confirm the number of operations in a transaction is only 1 at the commit phase, if the single operation feature is enabled on the transaction?

@komamitsu The single-operation flag is set to true only for transactions that are executed without explicitly beginning a transaction, such as the following methods:

DistributedTransactionManger transactionManager = ...;

Optional<Result> result = transactionManager.get(Get.newBuilder()...);

List<Result> results = transactionManager.scan(Scan.newBuilder()...);

Scanner scanner = transactionManager.getScanner(Scan.newBuilder()...);

transactionManager.insert(Insert.newBuilder()...);

transactionManager.upsert(Upsert.newBuilder()...);

transactionManager.update(Update.newBuilder()...);

transactionManager.delete(Delete.newBuilder()...);

transactionManager.mutate(Arrays.asList(Delete.newBuilder()..., Update.newBuilder()...));

In these cases, the number of operations in a transaction is always 1 at the commit phase (except for mutate, but we can handle it similarly). So we don’t need to explicitly check the number of operations.

komamitsu

LGTM! 👍

feeblefakie · 2025-06-17T04:04:40Z

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

            parallelExecutor,
-            readOnly);
+            readOnly,
+            singleOperation);


Suggested change

singleOperation);

oneOperation);

Fixed in 042d1e1. Thanks!

Torch3333

LGTM, thank you!

feeblefakie

LGTM! Thank you!

Further optimizations for Consensus Commit

fd2e415

brfrn169 requested a review from Copilot June 13, 2025 06:37

brfrn169 self-assigned this Jun 13, 2025

brfrn169 added the improvement label Jun 13, 2025

Copilot AI reviewed Jun 13, 2025

View reviewed changes

brfrn169 commented Jun 13, 2025

View reviewed changes

brfrn169 changed the title ~~Adds additional optimizations for Consensus Commit~~ Adds further optimizations for Consensus Commit Jun 13, 2025

brfrn169 requested review from Torch3333, feeblefakie and komamitsu June 13, 2025 07:00

brfrn169 changed the title ~~Adds further optimizations for Consensus Commit~~ Add further optimizations for Consensus Commit Jun 13, 2025

komamitsu reviewed Jun 16, 2025

View reviewed changes

Fix based on feedback

61b2860

feeblefakie reviewed Jun 16, 2025

View reviewed changes

brfrn169 requested a review from komamitsu June 16, 2025 04:40

komamitsu approved these changes Jun 16, 2025

View reviewed changes

Fix based on feedback

cefe83b

brfrn169 requested a review from feeblefakie June 16, 2025 05:40

feeblefakie reviewed Jun 17, 2025

View reviewed changes

Torch3333 approved these changes Jun 17, 2025

View reviewed changes

Rename single-operation to one-operation

042d1e1

brfrn169 requested a review from feeblefakie June 17, 2025 04:52

feeblefakie approved these changes Jun 18, 2025

View reviewed changes

Merge branch 'master' into further-optimizations-for-consensus-commit

9487c65

brfrn169 merged commit dfb51f5 into master Jun 18, 2025
55 checks passed

brfrn169 deleted the further-optimizations-for-consensus-commit branch June 18, 2025 03:36

feeblefakie pushed a commit that referenced this pull request Jun 18, 2025

Add further optimizations for Consensus Commit (#2764)

aa4905f

feeblefakie mentioned this pull request Jun 18, 2025

Backport to branch(3) : Add further optimizations for Consensus Commit #2783

Merged

Add further optimizations for Consensus Commit #2764

Add further optimizations for Consensus Commit #2764

Uh oh!

Conversation

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

Choose a reason for hiding this comment

Uh oh!

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

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!

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

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

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!

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

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

brfrn169 commented Jun 13, 2025 •

edited

Loading

brfrn169 Jun 13, 2025 •

edited

Loading

brfrn169 commented Jun 16, 2025 •

edited

Loading