Optimize mutations based on the storage’s mutation atomicity unit in Consensus Commit #2807

brfrn169 · 2025-06-20T06:30:16Z

Description

This PR adds an optimization for handling mutations in Consensus Commit based on the storage’s mutation atomicity unit.

Currently, Consensus Commit always splits the necessary mutations by partition and executes them using the DistributedStorage.mutate() method. However, we can instead split the mutations based on the storage’s mutation atomicity unit and execute them accordingly. That’s the basic idea behind this optimization.

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

Added an optimization to Consensus Commit that splits mutations based on the storage’s mutation atomicity unit, improving efficiency and performance in certain storage implementations.

…n atomicity unit

Copilot

Pull Request Overview

This PR replaces the existing PartitionedMutations approach with a new MutationsGrouper that batches storage mutations according to each storage’s mutation atomicity unit, and updates all call sites accordingly.

Introduce MutationsGrouper and wire it into CommitHandler, CommitHandlerWithGroupCommit, ConsensusCommitManager, and TwoPhaseConsensusCommitManager
Remove the old PartitionedMutations class and its tests
Update integration tests to verify the number of storage.mutate(...) calls based on the configured atomicity unit

Reviewed Changes

Copilot reviewed 13 out of 13 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
integration-test/.../ConsensusCommitSpecificIntegrationTestBase.java	Switch on `StorageInfo.getMutationAtomicityUnit()` in two existing tests
integration-test/.../ConsensusCommitNullMetadataIntegrationTestBase.java	Instantiate and pass `MutationsGrouper` in `createCommitHandler`
core/.../PartitionedMutationsTest.java	Deleted obsolete tests for the removed `PartitionedMutations` class
core/.../PartitionedMutationsKeyTest.java	Deleted obsolete tests for the removed `PartitionedMutations.Key`
core/.../MutationsGrouperTest.java	Added comprehensive tests for new `MutationsGrouper`
core/.../CommitHandlerWithGroupCommitTest.java	Updated test helper to include `MutationsGrouper`
core/.../CommitHandlerTest.java	Inject and mock `StorageInfoProvider` and `MutationsGrouper`
core/.../TwoPhaseConsensusCommitManager.java	Pass `MutationsGrouper` into 2PC manager constructor
core/.../PartitionedMutations.java	Removed retired class
core/.../MutationsGrouper.java	New class that groups and batches mutations based on atomicity unit
core/.../ConsensusCommitManager.java	Wire in `MutationsGrouper` in commit handler factory
core/.../CommitHandlerWithGroupCommit.java	Add `MutationsGrouper` field and constructor parameter
core/.../CommitHandler.java	Replace `PartitionedMutations` with `MutationsGrouper.groupMutations`

Comments suppressed due to low confidence (2)

integration-test/src/main/java/com/scalar/db/transaction/consensuscommit/ConsensusCommitSpecificIntegrationTestBase.java:7189

[nitpick] This test method name is extremely long and reduces readability. Consider shortening it to clearly state the scenario and expected behavior—e.g., putAndCommit_varyingAtomicity_shouldBatchMutationsCorrectly().

      putAndCommit_SinglePartitionMutationsGiven_ShouldBehaveCorrectlyBasedOnStorageMutationAtomicityUnit(

core/src/test/java/com/scalar/db/transaction/consensuscommit/CommitHandlerWithGroupCommitTest.java:70

The variable storageInfoProvider is not declared in this test class. Add @Mock protected StorageInfoProvider storageInfoProvider; and initialize mocks (e.g., via MockitoAnnotations.openMocks(this);) before using it.

        new MutationsGrouper(storageInfoProvider),

Copilot · 2025-06-20T06:32:21Z

core/src/test/java/com/scalar/db/transaction/consensuscommit/CommitHandlerTest.java

  @BeforeEach
  void setUp() throws Exception {
+    MockitoAnnotations.openMocks(this).close();


Calling .close() immediately after openMocks tears down the mocks before the test runs. Instead, store the returned AutoCloseable in a field and close it in an @AfterEach method, or omit .close() here.

Suggested change

@BeforeEach

void setUp() throws Exception {

MockitoAnnotations.openMocks(this).close();

private AutoCloseable autoCloseableMocks;

@BeforeEach

void setUp() throws Exception {

autoCloseableMocks = MockitoAnnotations.openMocks(this);

Copilot · 2025-06-20T06:32:22Z

core/src/test/java/com/scalar/db/transaction/consensuscommit/MutationsGrouperTest.java

+
+  @BeforeEach
+  public void setUp() throws Exception {
+    MockitoAnnotations.openMocks(this).close();
+
+    grouper = new MutationsGrouper(storageInfoProvider);
+  }
+


Similar to other tests, closing the Mockito session immediately will deactivate mocks. Keep the AutoCloseable returned by openMocks in a field and close in an @AfterEach.

Suggested change

@BeforeEach

public void setUp() throws Exception {

MockitoAnnotations.openMocks(this).close();

grouper = new MutationsGrouper(storageInfoProvider);

}

private AutoCloseable mocks;

@BeforeEach

public void setUp() throws Exception {

mocks = MockitoAnnotations.openMocks(this);

grouper = new MutationsGrouper(storageInfoProvider);

}

@AfterEach

public void tearDown() throws Exception {

if (mocks != null) {

mocks.close();

}

}

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

brfrn169 · 2025-06-20T06:39:23Z

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

+import javax.annotation.concurrent.ThreadSafe;
+
+@ThreadSafe
+public class MutationsGrouper {


Added MutationsGrouper that groups the mutations by based on the storage’s mutation atomicity unit.

brfrn169 · 2025-06-20T06:40:44Z

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

          new PrepareMutationComposer(snapshot.getId(), tableMetadataManager);
      snapshot.to(composer);
-      PartitionedMutations mutations = new PartitionedMutations(composer.get());
+      List<List<Mutation>> groupedMutations = mutationsGrouper.groupMutations(composer.get());


Split the mutations for prepare-records.

brfrn169 · 2025-06-20T06:40:58Z

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

          new CommitMutationComposer(snapshot.getId(), tableMetadataManager);
      snapshot.to(composer);
-      PartitionedMutations mutations = new PartitionedMutations(composer.get());
+      List<List<Mutation>> groupedMutations = mutationsGrouper.groupMutations(composer.get());


Split the mutations for commit-records.

brfrn169 · 2025-06-20T06:41:10Z

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

          new RollbackMutationComposer(snapshot.getId(), storage, tableMetadataManager);
      snapshot.to(composer);
-      PartitionedMutations mutations = new PartitionedMutations(composer.get());
+      List<List<Mutation>> groupedMutations = mutationsGrouper.groupMutations(composer.get());


Split the mutations for rollback-records.

Torch3333

LGTM, thank you!

feeblefakie

LGTM! Thank you!

komamitsu · 2025-06-20T08:23:46Z

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

+  public List<List<Mutation>> groupMutations(Collection<Mutation> mutations)
+      throws ExecutionException {
+    // MutationGroup mutations by their storage info and atomicity unit
+    List<MutationGroup> groups = new ArrayList<>();


[minor] This can be removed by using groupToBatches.keySet() instead?

komamitsu · 2025-06-20T08:42:59Z

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

+      assert mutation.forNamespace().isPresent();
+      StorageInfo storageInfo = storageInfoProvider.getStorageInfo(mutation.forNamespace().get());
+
+      MutationGroup matchedGroup = null;


[minor] If MutationGroup.equals() has a similar logic to isSameGroup(), maybe these lines can be simplified like this?

MutationGroup group = new MutationGroup(mutation, storageInfo); List<List<Mutation>> batches = groupToBatches.computeIfAbsent(group, g -> new ArrayList<>()); int maxCount = group.storageInfo.getMaxAtomicMutationsCount();

You are right. Fixed in b44e397. Thanks!

…on-atomicity-unit

komamitsu

LGTM! 👍

…Consensus Commit (#2807)

Optimize mutations in Consensus Commit based on the storage’s mutatio…

4fd6625

…n atomicity unit

brfrn169 requested a review from Copilot June 20, 2025 06:30

brfrn169 self-assigned this Jun 20, 2025

brfrn169 added the improvement label Jun 20, 2025

brfrn169 changed the title ~~Optimize mutations in Consensus Commit based on the storage’s mutation atomicity unit~~ Optimize mutations based on the storage’s mutation atomicity unit in Consensus Commit Jun 20, 2025

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 06:41

Torch3333 approved these changes Jun 20, 2025

View reviewed changes

feeblefakie approved these changes Jun 20, 2025

View reviewed changes

komamitsu reviewed Jun 20, 2025

View reviewed changes

brfrn169 added 2 commits June 20, 2025 18:09

Fix based on feedback

b44e397

Merge branch 'master' into optimize-mutations-based-on-storage-mutati…

25727bf

…on-atomicity-unit

brfrn169 requested a review from komamitsu June 20, 2025 09:13

komamitsu approved these changes Jun 20, 2025

View reviewed changes

brfrn169 merged commit cd94a96 into master Jun 20, 2025
56 checks passed

brfrn169 deleted the optimize-mutations-based-on-storage-mutation-atomicity-unit branch June 20, 2025 09:33

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

Optimize mutations based on the storage’s mutation atomicity unit in …

613d0d3

…Consensus Commit (#2807)

feeblefakie mentioned this pull request Jun 20, 2025

Backport to branch(3) : Optimize mutations based on the storage’s mutation atomicity unit in Consensus Commit #2810

Merged

Optimize mutations based on the storage’s mutation atomicity unit in Consensus Commit #2807

Optimize mutations based on the storage’s mutation atomicity unit in Consensus Commit #2807

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!

Copilot AI Jun 20, 2025

Choose a reason for hiding this comment

Uh oh!

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

Choose a reason for hiding this comment

Uh oh!

brfrn169 Jun 20, 2025

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!

komamitsu Jun 20, 2025

Choose a reason for hiding this comment

Uh oh!

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

komamitsu 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