optional validation of immutable ts on read operations #3414
Conversation
|
I would propose we make the transaction managers handle this safely? You need to validate before bubbling up any exception, basically |
|
If we want to do that, we should wrap the |
There was a problem hiding this comment.
The changes as written make sense. Though as @sandorw notes explicitly above, there is a behaviour change if my transaction task contains inconsistent reads followed by internal validation, which we should address.
Also, I'm pretty sure we want release notes.
| return; | ||
| } | ||
| throwIfPreCommitRequirementsNotMet(null, timestamp); | ||
| } | ||
|
|
||
| private boolean isValidationNecessaryOnReads(TableReference tableRef) { | ||
| return isValidationNecessary(tableRef) && validateLocksOnReads; |
There was a problem hiding this comment.
probably premature, but if in most usage validateLocksOnReads is going to be false, then flipping the arguments makes sense.
| @@ -747,13 +750,17 @@ private static boolean isSingleton(Iterable<?> elements) { | |||
| }; | |||
| } | |||
|
|
|||
| private void validateExternalAndCommitLocksIfNecessary(TableReference tableRef, long timestamp) { | |||
| if (!isValidationNecessary(tableRef)) { | |||
| private void validateExternalAndCommitLocksOnReadIfNecessary(TableReference tableRef, long timestamp) { | |||
There was a problem hiding this comment.
nit: can we rename this to validatePreCommitRequirementsOnReadIfNecessary? Mainly because this checks the pre-commit conditions too, which may not be 'external locks'
| this.delegate = delegate; | ||
| this.timelockService = timelockService; | ||
| this.immutableTsLock = immutableTsLock; | ||
|
|
| /** | ||
| * Best effort attempt to keep backwards compatibility while making immutableTs lock validation optional on reads. | ||
| * Validating immutableTs lock only on commits rather than on every read may cause the TransactionTask to throw | ||
| * an unexpected non-retriable error as reads are not idempotent. This wrapper task will convert the exception thrown |
There was a problem hiding this comment.
Two issues:
- reads aren't idempotent (, because)
- if the immutableTs lock is lost, sweep may remove data our transaction is reading, affecting the consistency of the data read
| */ | ||
|
|
||
| package com.palantir.atlasdb.transaction.impl; | ||
|
|
| import com.palantir.lock.v2.LockToken; | ||
| import com.palantir.lock.v2.TimelockService; | ||
|
|
||
| public class LockCheckingTransactionTaskTest { |
Goals (and why):
Addressing this closed pr: #2996
Making lock validations optional for read operations.
Implementation Description (bullets):
Adding a configurable flag called
validateLocksOnReadsto TransactionManagers. It is set totrueby default to conserve previous behavior. Validation after a read operation is done only if table is thoroughly swept and this flag is set to true. Validation at commit time is done if table is thoroughly swept, independent ofvalidateLocksOnReadsset to true or false.Testing (What was existing testing like? What have you done to improve it?):
Adding two more test cases.
validateLocksOnReadsto true causes validation to run after a read operation, even if the transaction is not committed.validateLocksOnReadsto false. This time immutable ts locks are not checked after a read operation; but it is checked when we commit the transaction.I also tried to refactor tests a bit by creating a private method to generate SnapshotTransaction, as it is constructor is taking large number of parameters, and making it hard to read the test case. It still doesn't look great, maybe we can add a builder to be used only in tests?
Concerns (what feedback would you like?):
validateLocksOnReadsto false will cause giving up on idempotent read operations. Clients can have two different reads for same value in a transaction. This cannot cause a correctness problem, as transaction will fail at commit time; the way the transaction fails will change.Where should we start reviewing?:
SnapshotTransaction.javaPriority (whenever / two weeks / yesterday):
1 week?
This change is