Skip to content

Backport to branch(3.16) : Fix behavior of group commit with one-phase commit #2845

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jul 9, 2025
Merged

Backport to branch(3.16) : Fix behavior of group commit with one-phase commit #2845

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 17 additions & 0 deletions ...src/main/java/com/scalar/db/transaction/consensuscommit/CommitHandlerWithGroupCommit.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,23 @@ public void commit(Snapshot snapshot, boolean readOnly)
super.commit(snapshot, readOnly);
}

@Override
boolean canOnePhaseCommit(Snapshot snapshot) throws CommitException {
try {
return super.canOnePhaseCommit(snapshot);
} catch (CommitException e) {
cancelGroupCommitIfNeeded(snapshot.getId());
throw e;
}
Comment on lines +61 to +67
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot Jul 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Wrapping the call to super.canOnePhaseCommit in a try-catch block and then re-throwing the exception after cancelling the group commit might mask the original stack trace, making debugging harder. Consider logging the caught exception before re-throwing it to preserve the context.

Suggested change
boolean canOnePhaseCommit(Snapshot snapshot) throws CommitException {
try {
return super.canOnePhaseCommit(snapshot);
} catch (CommitException e) {
cancelGroupCommitIfNeeded(snapshot.getId());
throw e;
}
boolean canOnePhaseCommit(Snapshot snapshot) throws CommitException {
try {
return super.canOnePhaseCommit(snapshot);
} catch (CommitException e) {
logger.warn("canOnePhaseCommit failed", e); // Log the exception
cancelGroupCommitIfNeeded(snapshot.getId());
throw e;
}
}

}

@Override
void onePhaseCommitRecords(Snapshot snapshot)
throws CommitConflictException, UnknownTransactionStatusException {
cancelGroupCommitIfNeeded(snapshot.getId());
super.onePhaseCommitRecords(snapshot);
}

@Override
protected void onFailureBeforeCommit(Snapshot snapshot) {
cancelGroupCommitIfNeeded(snapshot.getId());
Expand Down
15 changes: 8 additions & 7 deletions core/src/test/java/com/scalar/db/transaction/consensuscommit/CommitHandlerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,11 @@ protected CommitHandler createCommitHandler(boolean coordinatorWriteOmissionOnRe
false);
}

protected CommitHandler createCommitHandlerWithOnePhaseCommit() {
return new CommitHandler(
storage, coordinator, tableMetadataManager, parallelExecutor, mutationsGrouper, true, true);
}
Comment on lines +93 to +96
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot Jul 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The createCommitHandlerWithOnePhaseCommit method is duplicated here and in CommitHandlerWithGroupCommitTest.java. Consider creating a single method in a common base class or utility class to avoid duplication and improve maintainability.


@BeforeEach
void setUp() throws Exception {
MockitoAnnotations.openMocks(this).close();
Expand Down Expand Up @@ -1068,8 +1073,9 @@ public void canOnePhaseCommit_WhenMutationsCannotBeGrouped_ShouldReturnFalse() t
}

@Test
public void canOnePhaseCommit_WhenMutationsGrouperThrowsException_ShouldThrowCommitException()
throws ExecutionException {
public void
canOnePhaseCommit_WhenMutationsGrouperThrowsExecutionException_ShouldThrowCommitException()
throws ExecutionException {
// Arrange
CommitHandler handler = createCommitHandlerWithOnePhaseCommit();
Snapshot snapshot = prepareSnapshot();
Expand Down Expand Up @@ -1180,11 +1186,6 @@ public void commit_OnePhaseCommitted_ShouldNotThrowAnyException()
verify(handler).onFailureBeforeCommit(snapshot);
}

private CommitHandler createCommitHandlerWithOnePhaseCommit() {
return new CommitHandler(
storage, coordinator, tableMetadataManager, parallelExecutor, mutationsGrouper, true, true);
}

protected void doThrowExceptionWhenCoordinatorPutState(
TransactionState targetState, Class<? extends Exception> exceptionClass)
throws CoordinatorException {
Expand Down
147 changes: 105 additions & 42 deletions ...test/java/com/scalar/db/transaction/consensuscommit/CommitHandlerWithGroupCommitTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,14 +15,14 @@

import com.scalar.db.api.TransactionState;
import com.scalar.db.exception.storage.ExecutionException;
import com.scalar.db.exception.transaction.CommitConflictException;
import com.scalar.db.exception.transaction.CommitException;
import com.scalar.db.exception.transaction.UnknownTransactionStatusException;
import com.scalar.db.exception.transaction.ValidationConflictException;
import com.scalar.db.transaction.consensuscommit.CoordinatorGroupCommitter.CoordinatorGroupCommitKeyManipulator;
import com.scalar.db.util.groupcommit.GroupCommitConfig;
import java.util.List;
import java.util.UUID;
import org.junit.jupiter.api.Disabled;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.params.ParameterizedTest;
import org.junit.jupiter.params.provider.ValueSource;
Expand Down Expand Up @@ -75,6 +75,20 @@ protected CommitHandler createCommitHandler(boolean coordinatorWriteOmissionOnRe
groupCommitter);
}

@Override
protected CommitHandler createCommitHandlerWithOnePhaseCommit() {
createGroupCommitterIfNotExists();
return new CommitHandlerWithGroupCommit(
storage,
coordinator,
tableMetadataManager,
parallelExecutor,
mutationsGrouper,
true,
true,
groupCommitter);
}

private String anyGroupCommitParentId() {
return parentKey;
}
Expand Down Expand Up @@ -199,72 +213,121 @@ public void commit_NoReadsInSnapshot_ShouldNotValidateRecords(boolean withSnapsh
verify(groupCommitter, never()).remove(anyId());
}

@Disabled("Enabling both one-phase commit and group commit is not supported")
@Override
@Test
public void canOnePhaseCommit_WhenOnePhaseCommitDisabled_ShouldReturnFalse() {}

@Disabled("Enabling both one-phase commit and group commit is not supported")
@Override
@Test
public void canOnePhaseCommit_WhenValidationRequired_ShouldReturnFalse() {}
public void onePhaseCommitRecords_WhenSuccessful_ShouldMutateUsingComposerMutations()
throws CommitConflictException, UnknownTransactionStatusException, ExecutionException {
super.onePhaseCommitRecords_WhenSuccessful_ShouldMutateUsingComposerMutations();

@Disabled("Enabling both one-phase commit and group commit is not supported")
@Override
@Test
public void canOnePhaseCommit_WhenNoWritesAndDeletes_ShouldReturnFalse() {}
// Assert
verify(groupCommitter).remove(anyId());
Comment on lines 216 to +223
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot Jul 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The remove method of groupCommitter is called, but the return value is not checked. If remove fails, it might indicate an issue that should be handled. Consider logging a warning if remove returns false.

    verify(groupCommitter).remove(anyId());
  }

}

@Disabled("Enabling both one-phase commit and group commit is not supported")
@Override
@Test
public void canOnePhaseCommit_WhenDeleteWithoutExistingRecord_ShouldReturnFalse() {}

@Disabled("Enabling both one-phase commit and group commit is not supported")
@Override
@Test
public void canOnePhaseCommit_WhenMutationsCanBeGrouped_ShouldReturnTrue() {}
public void
onePhaseCommitRecords_WhenNoMutationExceptionThrown_ShouldThrowCommitConflictException()
throws ExecutionException {
super.onePhaseCommitRecords_WhenNoMutationExceptionThrown_ShouldThrowCommitConflictException();

@Disabled("Enabling both one-phase commit and group commit is not supported")
@Override
@Test
public void canOnePhaseCommit_WhenMutationsCannotBeGrouped_ShouldReturnFalse() {}
// Assert
verify(groupCommitter).remove(anyId());
Comment on lines 226 to +234
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot Jul 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The remove method of groupCommitter is called, but the return value is not checked. If remove fails, it might indicate an issue that should be handled. Consider logging a warning if remove returns false.

    verify(groupCommitter).remove(anyId());
  }

}

@Disabled("Enabling both one-phase commit and group commit is not supported")
@Override
@Test
public void canOnePhaseCommit_WhenMutationsGrouperThrowsException_ShouldThrowCommitException() {}
@Override
public void
onePhaseCommitRecords_WhenRetriableExecutionExceptionThrown_ShouldThrowCommitConflictException()
throws ExecutionException {
super
.onePhaseCommitRecords_WhenRetriableExecutionExceptionThrown_ShouldThrowCommitConflictException();

@Disabled("Enabling both one-phase commit and group commit is not supported")
// Assert
verify(groupCommitter).remove(anyId());
Comment on lines 237 to +246
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot Jul 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The remove method of groupCommitter is called, but the return value is not checked. If remove fails, it might indicate an issue that should be handled. Consider logging a warning if remove returns false.

    verify(groupCommitter).remove(anyId());
  }

}

@Test
@Override
public void
onePhaseCommitRecords_WhenExecutionExceptionThrown_ShouldThrowUnknownTransactionStatusException()
throws ExecutionException {
super
.onePhaseCommitRecords_WhenExecutionExceptionThrown_ShouldThrowUnknownTransactionStatusException();

// Assert
verify(groupCommitter).remove(anyId());
Comment on lines +248 to +258
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot Jul 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The remove method of groupCommitter is called, but the return value is not checked. If remove fails, it might indicate an issue that should be handled. Consider logging a warning if remove returns false.

    verify(groupCommitter).remove(anyId());
  }

}

@Test
public void onePhaseCommitRecords_WhenSuccessful_ShouldMutateUsingComposerMutations() {}
@Override
public void canOnePhaseCommit_WhenOnePhaseCommitDisabled_ShouldReturnFalse() throws Exception {
super.canOnePhaseCommit_WhenOnePhaseCommitDisabled_ShouldReturnFalse();
groupCommitter.remove(anyId());
}
Comment on lines 261 to +266
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot Jul 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The remove method of groupCommitter is called, but the return value is not checked. If remove fails, it might indicate an issue that should be handled. Consider logging a warning if remove returns false.

    groupCommitter.remove(anyId());
  }


@Disabled("Enabling both one-phase commit and group commit is not supported")
@Test
@Override
public void canOnePhaseCommit_WhenValidationRequired_ShouldReturnFalse() throws Exception {
super.canOnePhaseCommit_WhenValidationRequired_ShouldReturnFalse();
groupCommitter.remove(anyId());
}
Comment on lines +268 to +273
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot Jul 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The remove method of groupCommitter is called, but the return value is not checked. If remove fails, it might indicate an issue that should be handled. Consider logging a warning if remove returns false.

    groupCommitter.remove(anyId());
  }


@Test
public void
onePhaseCommitRecords_WhenNoMutationExceptionThrown_ShouldThrowCommitConflictException() {}
@Override
public void canOnePhaseCommit_WhenNoWritesAndDeletes_ShouldReturnFalse() throws Exception {
super.canOnePhaseCommit_WhenNoWritesAndDeletes_ShouldReturnFalse();
groupCommitter.remove(anyId());
}
Comment on lines 275 to +280
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot Jul 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The remove method of groupCommitter is called, but the return value is not checked. If remove fails, it might indicate an issue that should be handled. Consider logging a warning if remove returns false.

    groupCommitter.remove(anyId());
  }


@Disabled("Enabling both one-phase commit and group commit is not supported")
@Test
@Override
public void canOnePhaseCommit_WhenDeleteWithoutExistingRecord_ShouldReturnFalse()
throws Exception {
super.canOnePhaseCommit_WhenDeleteWithoutExistingRecord_ShouldReturnFalse();
groupCommitter.remove(anyId());
Comment on lines +282 to +287
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot Jul 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The remove method of groupCommitter is called, but the return value is not checked. If remove fails, it might indicate an issue that should be handled. Consider logging a warning if remove returns false.

    groupCommitter.remove(anyId());
  }

}

@Test
public void
onePhaseCommitRecords_WhenRetriableExecutionExceptionThrown_ShouldThrowCommitConflictException() {}
@Override
public void canOnePhaseCommit_WhenMutationsCanBeGrouped_ShouldReturnTrue() throws Exception {
super.canOnePhaseCommit_WhenMutationsCanBeGrouped_ShouldReturnTrue();
groupCommitter.remove(anyId());
Comment on lines +289 to +294
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot Jul 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The remove method of groupCommitter is called, but the return value is not checked. If remove fails, it might indicate an issue that should be handled. Consider logging a warning if remove returns false.

    groupCommitter.remove(anyId());
  }

}

@Disabled("Enabling both one-phase commit and group commit is not supported")
@Test
@Override
public void canOnePhaseCommit_WhenMutationsCannotBeGrouped_ShouldReturnFalse() throws Exception {
super.canOnePhaseCommit_WhenMutationsCannotBeGrouped_ShouldReturnFalse();
groupCommitter.remove(anyId());
Comment on lines +297 to +301
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot Jul 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The remove method of groupCommitter is called, but the return value is not checked. If remove fails, it might indicate an issue that should be handled. Consider logging a warning if remove returns false.

    groupCommitter.remove(anyId());
  }

}

@Test
@Override
public void
onePhaseCommitRecords_WhenExecutionExceptionThrown_ShouldThrowUnknownTransactionStatusException() {}
canOnePhaseCommit_WhenMutationsGrouperThrowsExecutionException_ShouldThrowCommitException()
throws ExecutionException {
super
.canOnePhaseCommit_WhenMutationsGrouperThrowsExecutionException_ShouldThrowCommitException();

@Disabled("Enabling both one-phase commit and group commit is not supported")
@Override
@Test
public void commit_OnePhaseCommitted_ShouldNotThrowAnyException() {}
// Assert
verify(groupCommitter).remove(anyId());
Comment on lines 304 to +313
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot Jul 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The remove method of groupCommitter is called, but the return value is not checked. If remove fails, it might indicate an issue that should be handled. Consider logging a warning if remove returns false.

    verify(groupCommitter).remove(anyId());
  }

}

@Disabled("Enabling both one-phase commit and group commit is not supported")
@Test
@Override
public void commit_OnePhaseCommitted_ShouldNotThrowAnyException()
throws CommitException, UnknownTransactionStatusException {
super.commit_OnePhaseCommitted_ShouldNotThrowAnyException();
groupCommitter.remove(anyId());
}
Comment on lines +316 to +322
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot Jul 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The remove method of groupCommitter is called, but the return value is not checked. If remove fails, it might indicate an issue that should be handled. Consider logging a warning if remove returns false.

    groupCommitter.remove(anyId());
  }


@Test
@Override
public void
commit_OnePhaseCommitted_UnknownTransactionStatusExceptionThrown_ShouldThrowUnknownTransactionStatusException() {}
commit_OnePhaseCommitted_UnknownTransactionStatusExceptionThrown_ShouldThrowUnknownTransactionStatusException()
throws CommitException, UnknownTransactionStatusException {
super
.commit_OnePhaseCommitted_UnknownTransactionStatusExceptionThrown_ShouldThrowUnknownTransactionStatusException();
groupCommitter.remove(anyId());
}
Comment on lines 324 to +332
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot Jul 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The remove method of groupCommitter is called, but the return value is not checked. If remove fails, it might indicate an issue that should be handled. Consider logging a warning if remove returns false.

    groupCommitter.remove(anyId());
  }

}