[Caching] Check Validity of TransactionKeyValueService Before Commit

jeremyk-91 commented 3 months ago

General

Before this PR: We don't check that the TransactionKeyValueService is still valid at the commit timestamp - there exist circumstances where if we have a different TransactionKeyValueService responsible for the state of the world at a later commit timestamp, we want to prevent transactions that lie across this boundary from committing.

After this PR: ==COMMIT_MSG== TransactionKeyValueServices now declare validity and we check before a transaction commits that the TransactionKeyValueService is still valid. ==COMMIT_MSG==

Priority: High P2 - nothing specific but on critical path for an important workstream

Concerns / possible downsides (what feedback would you like?):

I opted to introduce this specifically before running the Serializable conflict check and any user pre-commit conditions, on the basis that the Serializable conflict check depends on being able to read a reasonable snapshot at the commit timestamp. A model that perhaps has fewer elements, but I think loses useful signal on why things are failing, is to treat this check as part of a pre-commit condition (but then the serializable check will run first, which means that around the falling edge of a migration where the source of truth moves from one TKVS to another, serializable conflicts that aren't actually serializable conflicts may fire.
The new test has some nasty plumbing, but I think the signal is worthwhile.
Is the SafeTransactionFailedRetryableException thing overkill? I would like telemetry for these exceptions, generally speaking.

Is documentation needed?: Not really.

Compatibility

Does this PR create any API breaks (e.g. at the Java or HTTP layers) - if so, do we have compatibility?: YES: TransactionKeyValueService now requires isValid() to be implemented. This is an internal API and so I believe fine.

Does this PR change the persisted format of any data - if so, do we have forward and backward compatibility?: No

The code in this PR may be part of a blue-green deploy. Can upgrades from previous versions safely coexist? (Consider restarts of blue or green nodes.): Yes

Does this PR rely on statements being true about other products at a deployment - if so, do we have correct product dependencies on these products (or other ways of verifying that these statements are true)?: No

Does this PR need a schema migration? No

Testing and Correctness

What, if any, assumptions are made about the current state of the world? If they change over time, how will we find out?: Nothing in particular

What was existing testing like? What have you done to improve it?: Added new tests.

If this PR contains complex concurrent or asynchronous code, is it correct? The onus is on the PR writer to demonstrate this.: No specific concurrency.

If this PR involves acquiring locks or other shared resources, how do we ensure that these are always released?: No locking.

Execution

How would I tell this PR works in production? (Metrics, logs, etc.): Nothing breaks, and once we start using this internally, that we see transactions get aborted and retry at the right time.

Has the safety of all log arguments been decided correctly?: I think so - the only args here are timestamps

Will this change significantly affect our spending on metrics or logs?: No

How would I tell that this PR does not work in production? (monitors, etc.): Either transactions across a TKVS change boundary still work, or normal transactions explode.

If this PR does not work as expected, how do I fix that state? Would rollback be straightforward?: Rollback

If the above plan is more complex than “recall and rollback”, please tag the support PoC here (if it is the end of the week, tag both the current and next PoC): N/A

Scale

Would this PR be expected to pose a risk at scale? Think of the shopping product at our largest stack.: I don't think so

Would this PR be expected to perform a large number of database calls, and/or expensive database calls (e.g., row range scans, concurrent CAS)?: No

Would this PR ever, with time and scale, become the wrong thing to do - and if so, how would we know that we need to do something differently?: Nothing specific comes to mind here.

Development Process

Where should we start reviewing?: SafeTransactionFailedRetriableException, then probably TKVS and co

If this PR is in excess of 500 lines excluding versions lock-files, why does it not make sense to split it?: It's not

Please tag any other people who should be aware of this PR: @jeremyk-91 @sverma30 @raiju

jkozlowski commented 3 months ago

Looking good, I have an internal PR ready

jeremyk-91 commented 3 months ago

👍 - Jakub cut an RC so his solo approval didn't count

svc-autorelease commented 3 months ago

Released 0.1064.0

palantir / atlasdb