Skip to content
This repository has been archived by the owner on Nov 14, 2024. It is now read-only.

Allow disabling validating locks on reads on a per transaction basis #7111

Merged
merged 6 commits into from
May 15, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -467,4 +467,30 @@ default void disableReadWriteConflictChecking(TableReference tableRef) {
default void markTableInvolved(TableReference tableRef) {
throw new UnsupportedOperationException();
}

/**
* Disables lock validation on reads.
* <p>
* This method should be called before any reads are done inside this transaction or after the last read that can
* result in a side effect.
* <p>
* This method is always safe to be called inside a transaction that has no side effects outside of transaction
* scope as necessary validation will still be executed at commit time.
*/
@RestrictedApi(
explanation = "This API is only meant to be used by AtlasDb proxies that want to make use of the "
+ "performance improvement that are achievable by avoiding immutable timestamp lock check on reads "
+ "and delaying them to commit time. When validation on reads is disabled, it is possible for a "
+ "transaction to read values that were thoroughly swept and the transaction would not fail until "
+ "validation is done at commit commit time. Disabling validation on reads in situations when a "
+ "transaction can potentially have side effects outside the transaction scope (e.g. remote call "
+ "to another service) can cause correctness issues. The API is restricted as misuses of it can "
+ "cause correctness issues.",
link = "https://github.com/palantir/atlasdb/pull/7111",
allowedOnPath = ".*/src/test/.*", // Unsafe behavior in tests is ok.
allowlistAnnotations = {ReviewedRestrictedApiUsage.class})
@Idempotent
default void disableValidatingLocksOnReads() {
throw new UnsupportedOperationException();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -204,6 +204,12 @@ public void disableReadWriteConflictChecking(TableReference tableRef) {
delegate().disableReadWriteConflictChecking(tableRef);
}

@ReviewedRestrictedApiUsage
@Override
public void disableValidatingLocksOnReads() {
delegate().disableValidatingLocksOnReads();
}

@Override
public void markTableInvolved(TableReference tableRef) {
delegate().markTableInvolved(tableRef);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@
import com.google.common.util.concurrent.ListenableFuture;
import com.google.common.util.concurrent.MoreExecutors;
import com.google.errorprone.annotations.MustBeClosed;
import com.google.errorprone.annotations.RestrictedApi;
import com.palantir.atlasdb.AtlasDbConstants;
import com.palantir.atlasdb.AtlasDbMetricNames;
import com.palantir.atlasdb.AtlasDbPerformanceConstants;
Expand Down Expand Up @@ -93,6 +94,7 @@
import com.palantir.atlasdb.transaction.api.TransactionLockTimeoutException;
import com.palantir.atlasdb.transaction.api.TransactionReadSentinelBehavior;
import com.palantir.atlasdb.transaction.api.ValueAndChangeMetadata;
import com.palantir.atlasdb.transaction.api.annotations.ReviewedRestrictedApiUsage;
import com.palantir.atlasdb.transaction.api.exceptions.MoreCellsPresentThanExpectedException;
import com.palantir.atlasdb.transaction.api.exceptions.SafeTransactionFailedRetriableException;
import com.palantir.atlasdb.transaction.api.expectations.ExpectationsData;
Expand Down Expand Up @@ -281,7 +283,7 @@ private enum State {
protected final DeleteExecutor deleteExecutor;
private final Timer.Context transactionTimerContext;
protected final TransactionOutcomeMetrics transactionOutcomeMetrics;
protected final boolean validateLocksOnReads;
protected volatile boolean validateLocksOnReads;
protected final Supplier<TransactionConfig> transactionConfig;
protected final TableLevelMetricsController tableLevelMetricsController;
protected final SuccessCallbackManager successCallbackManager = new SuccessCallbackManager();
Expand Down Expand Up @@ -433,6 +435,24 @@ public void disableReadWriteConflictChecking(TableReference tableRef) {
conflictDetectionManager.disableReadWriteConflict(tableRef);
}

@RestrictedApi(
explanation = "This API is only meant to be used by AtlasDb proxies that want to make use of the "
+ "performance improvement that are achievable by avoiding immutable timestamp lock check on reads "
+ "and delaying them to commit time. When validation on reads is disabled, it is possible for a "
+ "transaction to read values that were thoroughly swept and the transaction would not fail until "
+ "validation is done at commit commit time. Disabling validation on reads in situations when a "
+ "transaction can potentially have side effects outside the transaction scope (e.g. remote call "
+ "to another service) can cause correctness issues. The API is restricted as misuses of it can "
+ "cause correctness issues.",
link = "https://github.com/palantir/atlasdb/pull/7111",
allowedOnPath = ".*/src/test/.*",
allowlistAnnotations = {ReviewedRestrictedApiUsage.class})
@Override
public void disableValidatingLocksOnReads() {
this.validateLocksOnReads = false;
this.readSnapshotValidator.disableValidatingLocksOnReads();
}

@Override
public void markTableInvolved(TableReference tableRef) {
// Not setting hasReads on purpose.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@

public final class DefaultReadSnapshotValidator implements ReadSnapshotValidator {
private final PreCommitRequirementValidator preCommitRequirementValidator;
private final boolean validateLocksOnReads;
private volatile boolean validateLocksOnReads;
private final SweepStrategyManager sweepStrategyManager;
private final Supplier<TransactionConfig> transactionConfigSupplier;

Expand Down Expand Up @@ -59,6 +59,11 @@ public boolean doesTableRequirePreCommitValidation(TableReference tableRef, bool
return requiresImmutableTimestampLocking(tableRef, allReadsCompleteOrValidated);
}

@Override
public void disableValidatingLocksOnReads() {
validateLocksOnReads = false;
}

private boolean isValidationNecessaryOnReads(TableReference tableRef, boolean allPossibleCellsReadAndPresent) {
return validateLocksOnReads && requiresImmutableTimestampLocking(tableRef, allPossibleCellsReadAndPresent);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,8 @@ ValidationState throwIfPreCommitRequirementsNotMetOnRead(

boolean doesTableRequirePreCommitValidation(TableReference tableRef, boolean allPossibleCellsReadAndPresent);

void disableValidatingLocksOnReads();

enum ValidationState {
COMPLETELY_VALIDATED,
NOT_COMPLETELY_VALIDATED
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1685,6 +1685,36 @@ public void validateLocksOnCommitIfEmptyReadsFollowedByNonEmptyReadsIfValidation
assertThatExceptionOfType(TransactionLockTimeoutException.class).isThrownBy(transaction::commit);
}

@Test
public void validateLocksOnCommitIfValidationFlagIsDisabledBeforeFirstRead() {
long transactionTs = timelockService.getFreshTimestamp();
LockImmutableTimestampResponse res = timelockService.lockImmutableTimestamp();

Transaction transaction =
getSnapshotTransactionWith(timelockService, () -> transactionTs, res, PreCommitConditions.NO_OP, true);
transaction.disableValidatingLocksOnReads();

timelockService.unlock(ImmutableSet.of(res.getLock()));
transaction.get(TABLE_SWEPT_THOROUGH, ImmutableSet.of(TEST_CELL));

assertThatExceptionOfType(TransactionLockTimeoutException.class).isThrownBy(transaction::commit);
}

@Test
public void doNotValidateLocksOnCommitIfValidationFlagIsDisabledAfterReads() {
long transactionTs = timelockService.getFreshTimestamp();
LockImmutableTimestampResponse res = timelockService.lockImmutableTimestamp();

Transaction transaction =
getSnapshotTransactionWith(timelockService, () -> transactionTs, res, PreCommitConditions.NO_OP, true);
transaction.get(TABLE_SWEPT_THOROUGH, ImmutableSet.of(TEST_CELL));

transaction.disableValidatingLocksOnReads();
timelockService.unlock(ImmutableSet.of(res.getLock()));

transaction.commit();
}

@Test
public void skipImmutableTimestampLockCheckIfReadingEqualsExpectedNumberOfValuesEvenWhenRequestedMore() {
putCellsInTable(List.of(TEST_CELL), TABLE_SWEPT_THOROUGH);
Expand Down
5 changes: 5 additions & 0 deletions changelog/@unreleased/pr-7111.v2.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
type: feature
feature:
description: Users can disable lock validation for reads on a per transaction basis.
links:
- https://github.com/palantir/atlasdb/pull/7111