From 583e082d02ddd976408eae9612a4c540b98dc46d Mon Sep 17 00:00:00 2001 From: Stevan Ognjanovic Date: Mon, 6 May 2024 18:27:18 +0100 Subject: [PATCH 1/6] add method to disable read validation --- .../atlasdb/transaction/api/Transaction.java | 14 ++++++++++++++ .../transaction/impl/ForwardingTransaction.java | 5 +++++ .../transaction/impl/SnapshotTransaction.java | 8 +++++++- .../precommit/DefaultReadSnapshotValidator.java | 7 ++++++- .../impl/precommit/ReadSnapshotValidator.java | 2 ++ 5 files changed, 34 insertions(+), 2 deletions(-) diff --git a/atlasdb-api/src/main/java/com/palantir/atlasdb/transaction/api/Transaction.java b/atlasdb-api/src/main/java/com/palantir/atlasdb/transaction/api/Transaction.java index 5baf33731cb..89572af7a8b 100644 --- a/atlasdb-api/src/main/java/com/palantir/atlasdb/transaction/api/Transaction.java +++ b/atlasdb-api/src/main/java/com/palantir/atlasdb/transaction/api/Transaction.java @@ -467,4 +467,18 @@ default void disableReadWriteConflictChecking(TableReference tableRef) { default void markTableInvolved(TableReference tableRef) { throw new UnsupportedOperationException(); } + + /** + * Disables lock validation on reads. + *

+ * 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. + *

+ * 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. + */ + @Idempotent + default void disableValidatingLocksOnReads() { + throw new UnsupportedOperationException(); + } } diff --git a/atlasdb-client/src/main/java/com/palantir/atlasdb/transaction/impl/ForwardingTransaction.java b/atlasdb-client/src/main/java/com/palantir/atlasdb/transaction/impl/ForwardingTransaction.java index e29d43ff9c4..133cb643d2f 100644 --- a/atlasdb-client/src/main/java/com/palantir/atlasdb/transaction/impl/ForwardingTransaction.java +++ b/atlasdb-client/src/main/java/com/palantir/atlasdb/transaction/impl/ForwardingTransaction.java @@ -204,6 +204,11 @@ public void disableReadWriteConflictChecking(TableReference tableRef) { delegate().disableReadWriteConflictChecking(tableRef); } + @Override + public void disableValidatingLocksOnReads() { + delegate().disableValidatingLocksOnReads(); + } + @Override public void markTableInvolved(TableReference tableRef) { delegate().markTableInvolved(tableRef); diff --git a/atlasdb-impl-shared/src/main/java/com/palantir/atlasdb/transaction/impl/SnapshotTransaction.java b/atlasdb-impl-shared/src/main/java/com/palantir/atlasdb/transaction/impl/SnapshotTransaction.java index e3bf834e908..5a629c027bc 100644 --- a/atlasdb-impl-shared/src/main/java/com/palantir/atlasdb/transaction/impl/SnapshotTransaction.java +++ b/atlasdb-impl-shared/src/main/java/com/palantir/atlasdb/transaction/impl/SnapshotTransaction.java @@ -281,7 +281,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; protected final TableLevelMetricsController tableLevelMetricsController; protected final SuccessCallbackManager successCallbackManager = new SuccessCallbackManager(); @@ -433,6 +433,12 @@ public void disableReadWriteConflictChecking(TableReference tableRef) { conflictDetectionManager.disableReadWriteConflict(tableRef); } + @Override + public synchronized void disableValidatingLocksOnReads() { + this.validateLocksOnReads = false; + this.readSnapshotValidator.disableValidatingLocksOnReads(); + } + @Override public void markTableInvolved(TableReference tableRef) { // Not setting hasReads on purpose. diff --git a/atlasdb-impl-shared/src/main/java/com/palantir/atlasdb/transaction/impl/precommit/DefaultReadSnapshotValidator.java b/atlasdb-impl-shared/src/main/java/com/palantir/atlasdb/transaction/impl/precommit/DefaultReadSnapshotValidator.java index 73c46dd5d9a..0a547b796c5 100644 --- a/atlasdb-impl-shared/src/main/java/com/palantir/atlasdb/transaction/impl/precommit/DefaultReadSnapshotValidator.java +++ b/atlasdb-impl-shared/src/main/java/com/palantir/atlasdb/transaction/impl/precommit/DefaultReadSnapshotValidator.java @@ -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 transactionConfigSupplier; @@ -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); } diff --git a/atlasdb-impl-shared/src/main/java/com/palantir/atlasdb/transaction/impl/precommit/ReadSnapshotValidator.java b/atlasdb-impl-shared/src/main/java/com/palantir/atlasdb/transaction/impl/precommit/ReadSnapshotValidator.java index cbdadd51d84..46d1c06ed4f 100644 --- a/atlasdb-impl-shared/src/main/java/com/palantir/atlasdb/transaction/impl/precommit/ReadSnapshotValidator.java +++ b/atlasdb-impl-shared/src/main/java/com/palantir/atlasdb/transaction/impl/precommit/ReadSnapshotValidator.java @@ -41,6 +41,8 @@ ValidationState throwIfPreCommitRequirementsNotMetOnRead( boolean doesTableRequirePreCommitValidation(TableReference tableRef, boolean allPossibleCellsReadAndPresent); + void disableValidatingLocksOnReads(); + enum ValidationState { COMPLETELY_VALIDATED, NOT_COMPLETELY_VALIDATED From 4e4415fa4760b1a050ad0d20e95c6fc6d4c9b9f5 Mon Sep 17 00:00:00 2001 From: Stevan Ognjanovic Date: Mon, 6 May 2024 18:46:52 +0100 Subject: [PATCH 2/6] add tests --- .../impl/AbstractSnapshotTransactionTest.java | 30 +++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/atlasdb-tests-shared/src/test/java/com/palantir/atlasdb/transaction/impl/AbstractSnapshotTransactionTest.java b/atlasdb-tests-shared/src/test/java/com/palantir/atlasdb/transaction/impl/AbstractSnapshotTransactionTest.java index e32451dabe3..801d0b34872 100644 --- a/atlasdb-tests-shared/src/test/java/com/palantir/atlasdb/transaction/impl/AbstractSnapshotTransactionTest.java +++ b/atlasdb-tests-shared/src/test/java/com/palantir/atlasdb/transaction/impl/AbstractSnapshotTransactionTest.java @@ -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); From 20bd86fde45a04f78e0b9c940cd64c98c9537417 Mon Sep 17 00:00:00 2001 From: Stevan Ognjanovic Date: Tue, 7 May 2024 22:08:38 +0100 Subject: [PATCH 3/6] add restricted API --- .../atlasdb/transaction/api/Transaction.java | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/atlasdb-api/src/main/java/com/palantir/atlasdb/transaction/api/Transaction.java b/atlasdb-api/src/main/java/com/palantir/atlasdb/transaction/api/Transaction.java index 89572af7a8b..4285083b3a6 100644 --- a/atlasdb-api/src/main/java/com/palantir/atlasdb/transaction/api/Transaction.java +++ b/atlasdb-api/src/main/java/com/palantir/atlasdb/transaction/api/Transaction.java @@ -477,6 +477,18 @@ default void markTableInvolved(TableReference tableRef) { * 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(); From b5efbc8ca75d466b6f2459c2021a99fa4d645b42 Mon Sep 17 00:00:00 2001 From: Stevan Ognjanovic Date: Tue, 7 May 2024 22:11:29 +0100 Subject: [PATCH 4/6] restricted api --- .../transaction/impl/ForwardingTransaction.java | 13 +++++++++++++ .../transaction/impl/SnapshotTransaction.java | 16 +++++++++++++++- 2 files changed, 28 insertions(+), 1 deletion(-) diff --git a/atlasdb-client/src/main/java/com/palantir/atlasdb/transaction/impl/ForwardingTransaction.java b/atlasdb-client/src/main/java/com/palantir/atlasdb/transaction/impl/ForwardingTransaction.java index 133cb643d2f..0fb116d0146 100644 --- a/atlasdb-client/src/main/java/com/palantir/atlasdb/transaction/impl/ForwardingTransaction.java +++ b/atlasdb-client/src/main/java/com/palantir/atlasdb/transaction/impl/ForwardingTransaction.java @@ -17,6 +17,7 @@ import com.google.common.collect.ForwardingObject; import com.google.common.util.concurrent.ListenableFuture; +import com.google.errorprone.annotations.RestrictedApi; import com.palantir.atlasdb.keyvalue.api.BatchColumnRangeSelection; import com.palantir.atlasdb.keyvalue.api.Cell; import com.palantir.atlasdb.keyvalue.api.ColumnRangeSelection; @@ -204,6 +205,18 @@ public void disableReadWriteConflictChecking(TableReference tableRef) { delegate().disableReadWriteConflictChecking(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() { delegate().disableValidatingLocksOnReads(); diff --git a/atlasdb-impl-shared/src/main/java/com/palantir/atlasdb/transaction/impl/SnapshotTransaction.java b/atlasdb-impl-shared/src/main/java/com/palantir/atlasdb/transaction/impl/SnapshotTransaction.java index 5a629c027bc..c80d6a352eb 100644 --- a/atlasdb-impl-shared/src/main/java/com/palantir/atlasdb/transaction/impl/SnapshotTransaction.java +++ b/atlasdb-impl-shared/src/main/java/com/palantir/atlasdb/transaction/impl/SnapshotTransaction.java @@ -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; @@ -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; @@ -433,8 +435,20 @@ 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 synchronized void disableValidatingLocksOnReads() { + public void disableValidatingLocksOnReads() { this.validateLocksOnReads = false; this.readSnapshotValidator.disableValidatingLocksOnReads(); } From c0916a764d251562fda82f5c214e01c01c7f9a25 Mon Sep 17 00:00:00 2001 From: svc-changelog Date: Tue, 7 May 2024 21:11:52 +0000 Subject: [PATCH 5/6] Add generated changelog entries --- changelog/@unreleased/pr-7111.v2.yml | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 changelog/@unreleased/pr-7111.v2.yml diff --git a/changelog/@unreleased/pr-7111.v2.yml b/changelog/@unreleased/pr-7111.v2.yml new file mode 100644 index 00000000000..3377ba5bd35 --- /dev/null +++ b/changelog/@unreleased/pr-7111.v2.yml @@ -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 From fe1b379e998fd8001ce5df04c20fea06040dda1a Mon Sep 17 00:00:00 2001 From: Stevan Ognjanovic Date: Tue, 7 May 2024 22:21:38 +0100 Subject: [PATCH 6/6] api usage --- .../transaction/impl/ForwardingTransaction.java | 14 +------------- 1 file changed, 1 insertion(+), 13 deletions(-) diff --git a/atlasdb-client/src/main/java/com/palantir/atlasdb/transaction/impl/ForwardingTransaction.java b/atlasdb-client/src/main/java/com/palantir/atlasdb/transaction/impl/ForwardingTransaction.java index 0fb116d0146..6caba9c9fcb 100644 --- a/atlasdb-client/src/main/java/com/palantir/atlasdb/transaction/impl/ForwardingTransaction.java +++ b/atlasdb-client/src/main/java/com/palantir/atlasdb/transaction/impl/ForwardingTransaction.java @@ -17,7 +17,6 @@ import com.google.common.collect.ForwardingObject; import com.google.common.util.concurrent.ListenableFuture; -import com.google.errorprone.annotations.RestrictedApi; import com.palantir.atlasdb.keyvalue.api.BatchColumnRangeSelection; import com.palantir.atlasdb.keyvalue.api.Cell; import com.palantir.atlasdb.keyvalue.api.ColumnRangeSelection; @@ -205,18 +204,7 @@ public void disableReadWriteConflictChecking(TableReference tableRef) { delegate().disableReadWriteConflictChecking(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}) + @ReviewedRestrictedApiUsage @Override public void disableValidatingLocksOnReads() { delegate().disableValidatingLocksOnReads();