Skip to content

Skip commit-state for read-only transactions in Consensus Commit #2765

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 7 commits into from
Jun 18, 2025
Merged

Skip commit-state for read-only transactions in Consensus Commit #2765

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
19aac3d
Skip commit-state for read-only transactions
brfrn169 May 22, 2025
e7cefba
Fix
brfrn169 Jun 14, 2025
6c6299d
Fix based on feedback
brfrn169 Jun 16, 2025
a40322c
Add a property `scalar.db.consensus_commit.coordinator.write_omission…
brfrn169 Jun 17, 2025
e783b13
Fix based on feedback
brfrn169 Jun 17, 2025
2c13b1c
[skip ci] Fix based on feedback
brfrn169 Jun 18, 2025
a303699
Merge branch 'master' into skip-commit-state-for-read-only-transactions
brfrn169 Jun 18, 2025
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
64 changes: 37 additions & 27 deletions core/src/main/java/com/scalar/db/transaction/consensuscommit/CommitHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,42 +106,52 @@ private void waitBeforePreparationSnapshotHookFuture(
}
}

public void commit(Snapshot snapshot) throws CommitException, UnknownTransactionStatusException {
public void commit(Snapshot snapshot, boolean readOnly)
throws CommitException, UnknownTransactionStatusException {
boolean hasNoWritesAndDeletesInSnapshot = readOnly || snapshot.hasNoWritesAndDeletes();

Optional<Future<Void>> snapshotHookFuture = invokeBeforePreparationSnapshotHook(snapshot);
try {
prepare(snapshot);
} catch (PreparationException e) {
safelyCallOnFailureBeforeCommit(snapshot);
abortState(snapshot.getId());
rollbackRecords(snapshot);
if (e instanceof PreparationConflictException) {
throw new CommitConflictException(e.getMessage(), e, e.getTransactionId().orElse(null));

if (!hasNoWritesAndDeletesInSnapshot) {
Copy link
Contributor

@feeblefakie feeblefakie Jun 16, 2025

Choose a reason for hiding this comment

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

I think double negative is a bit hard to read.
Since all the usage of hasNoWritesAndDeletesInSnapshot is with negation (!), how about making the method like hasSomeWritesAndDeletesInSnapshot so that we don't have to negate all the if clauses?

Copy link
Collaborator Author

@brfrn169 brfrn169 Jun 16, 2025

Choose a reason for hiding this comment

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

Fixed in 6c6299d. Thanks!

try {
prepare(snapshot);
} catch (PreparationException e) {
safelyCallOnFailureBeforeCommit(snapshot);
abortState(snapshot.getId());
rollbackRecords(snapshot);
if (e instanceof PreparationConflictException) {
throw new CommitConflictException(e.getMessage(), e, e.getTransactionId().orElse(null));
}
throw new CommitException(e.getMessage(), e, e.getTransactionId().orElse(null));
} catch (Exception e) {
safelyCallOnFailureBeforeCommit(snapshot);
throw e;
}
throw new CommitException(e.getMessage(), e, e.getTransactionId().orElse(null));
} catch (Exception e) {
safelyCallOnFailureBeforeCommit(snapshot);
throw e;
}
Copy link
Collaborator Author

@brfrn169 brfrn169 Jun 13, 2025

Choose a reason for hiding this comment

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

If no writes and deletes, skip prepare-records.


try {
validate(snapshot);
} catch (ValidationException e) {
safelyCallOnFailureBeforeCommit(snapshot);
abortState(snapshot.getId());
rollbackRecords(snapshot);
if (e instanceof ValidationConflictException) {
throw new CommitConflictException(e.getMessage(), e, e.getTransactionId().orElse(null));
if (!snapshot.hasNoReads()) {
Copy link
Contributor

@feeblefakie feeblefakie Jun 16, 2025

Choose a reason for hiding this comment

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

Ditto.

try {
validate(snapshot);
} catch (ValidationException e) {
safelyCallOnFailureBeforeCommit(snapshot);
abortState(snapshot.getId());
rollbackRecords(snapshot);
if (e instanceof ValidationConflictException) {
throw new CommitConflictException(e.getMessage(), e, e.getTransactionId().orElse(null));
}
throw new CommitException(e.getMessage(), e, e.getTransactionId().orElse(null));
} catch (Exception e) {
safelyCallOnFailureBeforeCommit(snapshot);
throw e;
}
throw new CommitException(e.getMessage(), e, e.getTransactionId().orElse(null));
} catch (Exception e) {
safelyCallOnFailureBeforeCommit(snapshot);
throw e;
}
Copy link
Collaborator Author

@brfrn169 brfrn169 Jun 13, 2025

Choose a reason for hiding this comment

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

If no reads, skip validate-record.


waitBeforePreparationSnapshotHookFuture(snapshot, snapshotHookFuture.orElse(null));

commitState(snapshot);
commitRecords(snapshot);
if (!hasNoWritesAndDeletesInSnapshot) {
commitState(snapshot);
commitRecords(snapshot);
}
Copy link
Collaborator Author

@brfrn169 brfrn169 Jun 13, 2025

Choose a reason for hiding this comment

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

If no writes and deletes, skip commit-state and commit-records.

}

protected void handleCommitConflict(Snapshot snapshot, Exception cause)
Expand Down
11 changes: 11 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 @@ -5,6 +5,7 @@
import com.scalar.db.api.DistributedStorage;
import com.scalar.db.api.TransactionState;
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.transaction.consensuscommit.Coordinator.State;
import com.scalar.db.util.groupcommit.Emittable;
Expand Down Expand Up @@ -37,6 +38,16 @@ public CommitHandlerWithGroupCommit(
this.groupCommitter = groupCommitter;
}

@Override
public void commit(Snapshot snapshot, boolean readOnly)
throws CommitException, UnknownTransactionStatusException {
if (!readOnly && snapshot.hasNoWritesAndDeletes()) {
cancelGroupCommitIfNeeded(snapshot.getId());
}

super.commit(snapshot, readOnly);
}

@Override
protected void onFailureBeforeCommit(Snapshot snapshot) {
cancelGroupCommitIfNeeded(snapshot.getId());
Expand Down
4 changes: 2 additions & 2 deletions core/src/main/java/com/scalar/db/transaction/consensuscommit/ConsensusCommit.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -269,7 +269,7 @@ public void commit() throws CommitException, UnknownTransactionStatusException {
CoreError.CONSENSUS_COMMIT_EXECUTING_IMPLICIT_PRE_READ_FAILED.buildMessage(), e, getId());
}

commit.commit(crud.getSnapshot());
commit.commit(crud.getSnapshot(), crud.isReadOnly());
}

@Override
Expand All @@ -280,7 +280,7 @@ public void rollback() {
logger.warn("Failed to close the scanner", e);
}

if (groupCommitter != null) {
if (groupCommitter != null && !crud.isReadOnly()) {
groupCommitter.remove(crud.getSnapshot().getId());
}
}
Expand Down
2 changes: 1 addition & 1 deletion core/src/main/java/com/scalar/db/transaction/consensuscommit/ConsensusCommitManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -221,7 +221,7 @@ DistributedTransaction beginReadOnly(Isolation isolation) {
DistributedTransaction begin(String txId, Isolation isolation, boolean readOnly) {
checkArgument(!Strings.isNullOrEmpty(txId));
checkNotNull(isolation);
if (isGroupCommitEnabled()) {
if (!readOnly && isGroupCommitEnabled()) {
assert groupCommitter != null;
txId = groupCommitter.reserve(txId);
}
Comment on lines +235 to 238
Copy link
Collaborator Author

@brfrn169 brfrn169 Jun 13, 2025

Choose a reason for hiding this comment

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

In read-only mode, skip reserving a slot in the group commit.

Expand Down
4 changes: 4 additions & 0 deletions core/src/main/java/com/scalar/db/transaction/consensuscommit/CrudHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -420,6 +420,10 @@ public Snapshot getSnapshot() {
return snapshot;
}

public boolean isReadOnly() {
return readOnly;
}

private interface ConsensusCommitScanner extends TransactionCrudOperable.Scanner {
boolean isClosed();
}
Expand Down
8 changes: 8 additions & 0 deletions core/src/main/java/com/scalar/db/transaction/consensuscommit/Snapshot.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -217,6 +217,14 @@ public boolean containsKeyInGetSet(Get get) {
return getSet.containsKey(get);
}

public boolean hasNoWritesAndDeletes() {
return writeSet.isEmpty() && deleteSet.isEmpty();
}

public boolean hasNoReads() {
return getSet.isEmpty() && scanSet.isEmpty() && scannerSet.isEmpty();
}

public Optional<TransactionResult> getResult(Key key) throws CrudException {
Optional<TransactionResult> result = readSet.getOrDefault(key, Optional.empty());
return mergeResult(key, result);
Expand Down
Loading