Handle KAE (or other exceptions of similar type) in the future when witnessing transactions #6533

Sam-Kramer · 2023-04-18T10:54:14Z

General

Before this PR:

After this PR:

==COMMIT_MSG==
Handles transactions that may or may have not been witnessed due to errors (such as KeyAlreadyExistsException) in the workload server.
==COMMIT_MSG==

Priority:

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

Is documentation needed?:

Compatibility

Does this PR create any API breaks (e.g. at the Java or HTTP layers) - if so, do we have compatibility?:

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

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.):

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)?:

Does this PR need a schema migration?

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?:

What was existing testing like? What have you done to improve it?:

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

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

Execution

How would I tell this PR works in production? (Metrics, logs, etc.):

Has the safety of all log arguments been decided correctly?:

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

How would I tell that this PR does not work in production? (monitors, etc.):

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

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):

Scale

Would this PR be expected to pose a risk at scale? Think of the shopping product at our largest stack.:

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)?:

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?:

Development Process

Where should we start reviewing?:

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

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

Sam-Kramer · 2023-04-18T10:55:29Z

...db-workload-server/src/main/java/com/palantir/atlasdb/workload/workflow/DefaultWorkflow.java

@@ -60,7 +61,7 @@ public static <T extends TransactionStore> Workflow create(
    @Override
    public WorkflowHistory run() {


We may wish to update the WorkflowHistory class that it only accepts FullyWitnessedTransaction rather than both classes. I think this is ok for now.

Yep. Could we leave a todo here? WorkflowHistory should only accept FullyWitnessedTransaction long term, yeah.

Sam-Kramer · 2023-04-18T10:56:12Z

...ad-server/src/test/java/com/palantir/atlasdb/workload/store/AtlasDbTransactionStoreTest.java

+        assertThat(onlyKeyAlreadyExistsThrowingStore.readWrite(List.of(writeTransactionAction)))
+                .hasValueSatisfying(witnessedTransaction -> {
+                    assertThat(witnessedTransaction).isInstanceOf(MaybeWitnessedTransaction.class);
+                    assertThat(witnessedTransaction.actions()).containsExactly(writeTransactionAction.witness());


re-reading this, probably should have checked that commit timestamp is present, but this is a precondition when we construct MaybeWitnessedTransaction, thus is not needed

jeremyk-91 · 2023-04-18T15:10:28Z

...main/java/com/palantir/atlasdb/workload/transaction/witnessed/MaybeWitnessedTransaction.java

+        Preconditions.checkArgument(
+                commitTimestamp().isPresent(),
+                "Commit timestamp must be present in a potentially witnessed transaction, as otherwise it is not"
+                        + " possible to validate whether or not a transaction has been committed.");


I think this can be stronger: something like given how the transaction protocol works, a transaction for which we haven't retrieved the commit timestamp has not written anything of significance to the database or something like that?

jeremyk-91 · 2023-04-18T15:11:29Z

...d-server-api/src/main/java/com/palantir/atlasdb/workload/store/ReadableTransactionStore.java

@@ -28,4 +28,9 @@ public interface ReadableTransactionStore {
     * @return The value of the cell for a given table.
     */
    Optional<Integer> get(String table, WorkloadCell cell);
+
+    /**
+     * Checks the startTimestamp for a given transaction whether it has committed.


nit: checks whether the transaction with the provided startTimestamp has committed?

jeremyk-91 · 2023-04-18T15:16:24Z

...tir/atlasdb/workload/transaction/witnessed/OnlyCommittedWitnessedTransactionVisitorTest.java

+
+    private static final MaybeWitnessedTransaction MAYBE_WITNESSED_TRANSACTION = MaybeWitnessedTransaction.builder()
+            .startTimestamp(100L)
+            .commitTimestamp(100L)


nit: let's try not to include situations that won't (generally!) happen - should we use 101L instead?

jeremyk-91 · 2023-04-18T15:17:33Z

...tir/atlasdb/workload/transaction/witnessed/OnlyCommittedWitnessedTransactionVisitorTest.java

+        lenient().when(transactionStore.isCommitted(anyLong())).thenThrow(new RuntimeException());
+        assertThat(READ_ONLY_WITNESSED_TRANSACTION.accept(
+                        new OnlyCommittedWitnessedTransactionVisitor(transactionStore)))
+                .contains(READ_ONLY_WITNESSED_TRANSACTION);


For readability, this kind of check is normally done by asserting verifyNoInteractions(transactionStore); (also, while it doesn't apply here, an implementation that catches exceptions and does stuff with them could pass this test as written!)

jeremyk-91 · 2023-04-18T15:20:41Z

...db-workload-server/src/main/java/com/palantir/atlasdb/workload/workflow/DefaultWorkflow.java

@@ -60,7 +61,7 @@ public static <T extends TransactionStore> Workflow create(
    @Override
    public WorkflowHistory run() {


Yep. Could we leave a todo here? WorkflowHistory should only accept FullyWitnessedTransaction long term, yeah.

jeremyk-91 · 2023-04-18T15:38:25Z

...rkload-server/src/main/java/com/palantir/atlasdb/workload/store/AtlasDbTransactionStore.java

+                    .startTimestamp(transaction.getTimestamp())
+                    .commitTimestamp(commitTimestampProvider
+                            .get()
+                            .getCommitTimestampOrThrowIfMaybeNotCommitted(transaction.getTimestamp()))


not actionable: This might work strangely if the transaction task deliberately throws a KeyAlreadyExistsException, though I don't think we need to worry about that

Later on, I made it the more general case of TransactionCommitFailedException, rather than just KAE, but still, a transaction could also throw this for funs. I think in this case we're ok, as we validate later on if it exists or not

jeremyk-91 · 2023-04-18T15:39:50Z

...rkload-server/src/main/java/com/palantir/atlasdb/workload/store/AtlasDbTransactionStore.java

+            log.info(
+                    "Failed to record transaction due to an exception for startTimestamp {}",
+                    SafeArg.of("startTimestamp", startTimestamp),
+                    e);


We might want to set the transaction reference at the beginning of the task, if we're concerned about something going wrong before we enter the commit-writes phase.

jeremyk-91 · 2023-04-18T15:42:48Z

...ad-server/src/test/java/com/palantir/atlasdb/workload/store/AtlasDbTransactionStoreTest.java

+        WriteTransactionAction writeTransactionAction =
+                WriteTransactionAction.of(TABLE_1, WORKLOAD_CELL_ONE, VALUE_ONE);
+        TransactionManager onlyThrowsKeyAlreadyExistsExceptionManager = spy(manager);
+        doAnswer(answer -> {


nit: Answer is a functional interface with a method that takes an InvocationOnMock, so this param should be named invocation or invocationOnMock

jeremyk-91 · 2023-04-18T15:45:25Z

...ad-server/src/test/java/com/palantir/atlasdb/workload/store/AtlasDbTransactionStoreTest.java

+                .runTaskWithConditionWithRetry(any(Supplier.class), any());
+        AtlasDbTransactionStore onlyKeyAlreadyExistsThrowingStore =
+                AtlasDbTransactionStore.create(onlyThrowsKeyAlreadyExistsExceptionManager, TABLES_TO_ATLAS_METADATA);
+        assertThat(onlyKeyAlreadyExistsThrowingStore.readWrite(List.of(writeTransactionAction)))


Hmm, I think the naming here is a bit hard to follow - not sure about the significance of the only. Maybe keyAlreadyExistsExceptionThrowingManager and keyAlreadyExistsExceptionThrowingStore?

jeremyk-91 · 2023-04-18T15:49:52Z

...main/java/com/palantir/atlasdb/workload/transaction/witnessed/MaybeWitnessedTransaction.java

+                .startTimestamp(startTimestamp())
+                .commitTimestamp(commitTimestamp())
+                .actions(actions())
+                .build();


non actionable: If we add an optional field to WitnessedTransaction after this, we won't copy over its value from the Maybe to the Fully version. I think this risk is ok. Maybe there's some clever refactoring we can do, but not needed here.

jeremyk-91

👍

Sam Kramer added 2 commits April 18, 2023 11:27

handle kae exceptions during execution

7113edf

make things prettier

81c62a6

Sam-Kramer added no changelog merge when ready labels Apr 18, 2023

Sam-Kramer commented Apr 18, 2023

View reviewed changes

Sam Kramer added 4 commits April 18, 2023 11:57

test name can be better

ca39726

some clean up

32dac63

fix class types

374053b

fix tests

23de549

jeremyk-91 reviewed Apr 18, 2023

View reviewed changes

Sam Kramer added 3 commits April 19, 2023 09:50

check entire cause chain for kae exceptions

37a96c8

catch all transaction commit failed exceptions

f8bf341

address nits

bc61f18

jeremyk-91 approved these changes May 3, 2023

View reviewed changes

Sam Kramer added 2 commits May 3, 2023 13:37

fix style

fca9cff

fix test

3737bcc

bulldozer-bot bot merged commit 2857ac2 into develop May 3, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle KAE (or other exceptions of similar type) in the future when witnessing transactions #6533

Handle KAE (or other exceptions of similar type) in the future when witnessing transactions #6533

Sam-Kramer commented Apr 18, 2023

Sam-Kramer Apr 18, 2023

jeremyk-91 Apr 18, 2023

Sam-Kramer Apr 18, 2023

jeremyk-91 Apr 18, 2023

jeremyk-91 Apr 18, 2023

jeremyk-91 Apr 18, 2023

jeremyk-91 Apr 18, 2023

jeremyk-91 Apr 18, 2023

jeremyk-91 Apr 18, 2023

Sam-Kramer May 3, 2023

jeremyk-91 Apr 18, 2023

jeremyk-91 Apr 18, 2023

jeremyk-91 Apr 18, 2023

jeremyk-91 Apr 18, 2023

jeremyk-91 left a comment

		@@ -60,7 +61,7 @@ public static <T extends TransactionStore> Workflow create(
		@Override
		public WorkflowHistory run() {

Handle KAE (or other exceptions of similar type) in the future when witnessing transactions #6533

Handle KAE (or other exceptions of similar type) in the future when witnessing transactions #6533

Conversation

Sam-Kramer commented Apr 18, 2023

General

Compatibility

Testing and Correctness

Execution

Scale

Development Process

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeremyk-91 left a comment

Choose a reason for hiding this comment