Add support for creating a FATE transaction by key #4204

cshannon · 2024-01-27T19:08:41Z

This updates FATE to support creating a transaction by a key which will be hashed to a transaction id. This can be used to guarantee only one transaction exists at the same time if the key is the same. Currently the key is just a byte array so anything can be used for the key. The key gets hashed to generate the transaction id (currently a long) and then encoded to be stored just like the existing randomly generated transaction ids.

I marked this as a draft PR for now to get some feedback as some changes are probably needed, in particular maybe changing the key from a byte array to something more strongly typed (see below) I added a few store tests but some tests are missing such as testing the collision detection. (Probably will require manually modifying some things in the store or ZK to simulate a collision or googling the algorithm to find two keys that cause a collision).

Something else is the hashing algorithm is using 128 bit murmur3 but converting to a positive long (since that's currently or tid) so the size is really only 63 bits and likely would lead to collisions at scale. Once the new FateId is added in #4191 we should explore changing the long tid into a byte array and using a 128 bit transaction id size. The algorithm could also be switched out to if desired (ie sha256)

For now the key is just a byte array to keep things simple but we can change it. The goal is to have unique fate operations a byte array is the most flexible as we can put anything into it. For example, if we wanted to create a key for a split operation for an extent we could serialize the operation name (ie split) and append it to the serialized KeyExtent. In the tests for now I'm only serializing the KeyExtent. We could create some utility methods to help with the serialization and deserialization.

However, we could also make the API more strongly typed and instead of a byte array as the key would create a new interface or class (maybe call it FateKey) and that could then contain an operation name and a KeyExtent and then handle the serialization. This could be nice as it would match up with the new FateId being created in #4191 but the downside is if we want different key types in the future it's not as flexible and we'd need to change or extend the API. If we just use an interface and had different key implementations it might be ok as we could add new key types in the future without changing the API.

This closes #4183

This updates FATE to support creating a transaction by a key which will be hashed to a transaction id. This can be used to guarantee only one transaction exists at the same time if the key is the same. Currently the key is just a byte array so anything can be used for the key. This closes apache#4183

core/src/main/java/org/apache/accumulo/core/fate/ZooStore.java

keith-turner · 2024-01-29T16:53:36Z

However, we could also make the API more strongly typed and instead of a byte array as the key would create a new interface or class (maybe call it FateKey) and that could then contain an operation name and a KeyExtent and then handle the serialization.

There will only be two use cases (split and compaction commit) for the fate key at the moment. So could have a class with static methods that support creating a FateKey object for the two uses case.

cshannon · 2024-01-30T11:41:50Z

There will only be two use cases (split and compaction commit) for the fate key at the moment. So could have a class with static methods that support creating a FateKey object for the two uses case.

That sounds good, we can expand it if we end up with more use cases in the future. I will work on re-working the byte array into a FateKey object this week as I think that is cleaner.

test/src/main/java/org/apache/accumulo/test/fate/accumulo/FateStoreIT.java

operation

This commit moves FateKey storage to the node for the ZooStore so it will be inserted atomically with the transaction id and also moves it to the TxColumnFamily for the Accumulo store.

FateKey

cshannon · 2024-02-04T21:00:05Z

@keith-turner - I think this is ready for another review. Since you last reviewed it I added the FateKey type instead of a byte array, I made FateKey create method atomic for ZK and added a conditional mutation for Accumulo and I updated the tests (including testing the collision detection

keith-turner

These changes look good. I am still reviewing but I am mostly done. May also need some changes in Fate class to do something like the following.

Attempt to seed a split operation with a repo
If the split exists with the given key and is not NEW then log a debug or trace. This is expected to happen when the system is busy and operations are not running and we keep seeding them
If there is a collision log an error

Not exactly sure how this should be done, trying to figure out case 2 above where we want to avoid spamming the logs w/ warn/error for an expected case. Also wondering what the new Fate method should be to use this new functionality in the store, maybe something like the following.

class Fate {
   public FateId startAndSeedTransaction(FateKey key, Repo repo) {
   }
}

The fate changes could be a separate PR or done in this PR.

test/src/main/java/org/apache/accumulo/test/fate/accumulo/FateStoreIT.java

keith-turner · 2024-02-07T00:58:57Z

test/src/main/java/org/apache/accumulo/test/fate/accumulo/FateStoreIT.java

+    // Replace the default hasing algorithm with one that always returns the same tid so
+    // we can check duplicate detection with different keys
+    executeTest(this::testCreateWithKeyCollision, AbstractFateStore.DEFAULT_MAX_DEFERRED,
+        (instanceType, fateKey) -> FateId.from(instanceType, 1000));


Nice way to test collisions.

test/src/main/java/org/apache/accumulo/test/fate/accumulo/FateStoreIT.java

core/src/main/java/org/apache/accumulo/core/logging/FateLogger.java

keith-turner · 2024-02-07T01:11:01Z

core/src/main/java/org/apache/accumulo/core/fate/accumulo/AccumuloStore.java

+    var status = newMutator(fateId).requireStatus().putStatus(TStatus.NEW).putKey(fateKey)
+        .putCreateTime(System.currentTimeMillis()).tryMutate();
+
+    // TODO: Any reason to retry here?


An unknown status means that a failure happened w/ the RPC for the conditional mutation and its not know if it went through. So need to scan in that case and see if it was created or not and possibly retry.

core/src/main/java/org/apache/accumulo/core/fate/AbstractFateStore.java

keith-turner · 2024-02-07T14:07:25Z

core/src/main/java/org/apache/accumulo/core/fate/AbstractFateStore.java

+      // to reserve/seed as long as the existing key is the same and not different as that would
+      // mean a collision
+    } else if (status == TStatus.NEW) {
+      Preconditions.checkState(tFateKey.isPresent(), "Tx key column is missing");


I don't think there is a test for a collision between a random fate id and hash derived fate id. Im 99.9% sure this code works, but was wondering if creating a test for that is worthwhile for detecting regressions.

@keith-turner - I just added a test for collision with key generated fate id and random id

The test works by creating the transaction using the key and then the test deletes the key outside of fate (either out of accumulo table or replaces the ZK node with an empty key). After removing the key the transaction appears to the store just like a normal random transaction so the test then verifies the failure when calling create again and also ensures the transaction can still be reserved and used since it was essentially "converted" to a random transaction.

keith-turner · 2024-02-08T02:22:29Z

Stubbed out possible interaction between fate and the fate store for keyed operations in #4178. Can see relevant changes in FateStore, Fate, and SplitTask

cshannon · 2024-02-08T14:38:59Z

Stubbed out possible interaction between fate and the fate store for keyed operations in #4178. Can see relevant changes in FateStore, Fate, and SplitTask

I'll take a look in detail tomorrow, I see you added Optional<FateTxStore<T>> createAndReserve(FateKey fateKey and that is something I will likely add to this PR when I update it. I think it makes sense to add it now vs a follow on

keith-turner · 2024-02-08T15:37:03Z

What lead to the createAndReserve call was trying to deal with the following race condition when I was trying to stub out using this feature.

class Fate {
  seed(..., FateKey key,...) {
       var fateId = create(key);
       // another thread could sneak in here
        var fateTxStore = store.reserve(fateId);
        try{
            // between create and reserve, its possible something happened with the key so now that we hold the reservation check that things are as expected
            store.checkKeyIsStillSame(fateId,key);
        }finally{
            fateTxStore.unreserve();
       }
   }

So hoping having the store create and reserve can avoid this.

Co-authored-by: Keith Turner <[email protected]>

this will make sure that if a transaction is created by a key it will reserve at the same time to prevent race conditions

cshannon · 2024-02-09T17:10:35Z

I made a bunch of updates and addressed everything and changed create(fateKey) to createAndReserve(fateKey) (which technically is implemented by reserving first in memory and then creating). I also added a test for verifying that collisions when using a key and with random ids work as expected.

…tus is not new

keith-turner · 2024-02-09T19:26:35Z

core/src/main/java/org/apache/accumulo/core/fate/AbstractFateStore.java

-        Preconditions.checkState(create(fateKey) != null,
-            "Unexpected null FateId when creating and reserving fateKey %s", fateKey);
+        if (create(fateKey).isPresent()) {
+          txStore = reservedTxStore;


Could modify the code to keep the return value of create and validate it.

Suggested change

txStore = reservedTxStore;

Precondition.checkState(fateId.equals(fateIdFromCreate));

txStore = reservedTxStore;

keith-turner · 2024-02-09T19:37:44Z

core/src/main/java/org/apache/accumulo/core/fate/AbstractFateStore.java

+        }
+      }
+    } else {
+      // Could not reserve so return empty


Created the following after seeing another log stmt in this PR. Thought it would be good to log something here like it does elsewhere. These trace stmts will be nice for tracking down bugs

Suggested change

// Could not reserve so return empty

// Could not reserve so return empty

log.trace("Another thread currently has transaction {} key {} reserved", fateId,

fateKey);

keith-turner · 2024-02-09T19:47:29Z

core/src/test/java/org/apache/accumulo/core/fate/TestStore.java


+  @Override
+  public Optional<FateTxStore<String>> createAndReserve(FateKey key) {
+    HashCode hashCode = Hashing.murmur3_128().hashBytes(key.getSerialized());


This TestStore is only used by unit test. If the existing unit test does not call thsi method then could throw an UnsupportedOpException.

cshannon requested a review from keith-turner January 27, 2024 19:08

cshannon self-assigned this Jan 27, 2024

keith-turner reviewed Jan 29, 2024

View reviewed changes

core/src/main/java/org/apache/accumulo/core/fate/ZooStore.java Outdated Show resolved Hide resolved

keith-turner reviewed Feb 1, 2024

View reviewed changes

test/src/main/java/org/apache/accumulo/test/fate/accumulo/FateStoreIT.java Show resolved Hide resolved

test/src/main/java/org/apache/accumulo/test/fate/accumulo/FateStoreIT.java Show resolved Hide resolved

cshannon added 6 commits February 2, 2024 08:15

Merge branch 'elasticity' into accumulo-4183

1771eb1

Updates after the change from tid to FateId

1e6a471

Create FateKey instead of a byte array for tracking the key for a fate

ec63c8b

operation

Make FateKey atomic

1646242

This commit moves FateKey storage to the node for the ZooStore so it will be inserted atomically with the transaction id and also moves it to the TxColumnFamily for the Accumulo store.

fix checkstyle

fe62cfb

Test updates and support configuring hashing algorithm for FateId from

5824aa9

FateKey

cshannon marked this pull request as ready for review February 4, 2024 20:58

keith-turner reviewed Feb 7, 2024

View reviewed changes

cshannon and others added 8 commits February 9, 2024 06:13

Merge branch 'elasticity' into accumulo-4183

ff4651a

Update FateLogger

9a73cf6

Co-authored-by: Keith Turner <[email protected]>

test improvements

27107b9

fix race condition in create(fateKey)

7793319

Add a test for a fate key collision with random fate id

8aad33e

error message and test updates

9fc782f

Update create(fateId,fateKey) to handle retry

3b2ca77

Replace store.create(key) with store.createAndReserve(key)

4f66736

this will make sure that if a transaction is created by a key it will reserve at the same time to prevent race conditions

Return empty optional for fate store instead of throwing error if sta…

2ad01d3

…tus is not new

keith-turner approved these changes Feb 9, 2024

View reviewed changes

cshannon added 2 commits February 9, 2024 15:18

Updates based on comments

ead1ccc

Don't call create twice

9dfa79c

cshannon merged commit 8417794 into apache:elasticity Feb 9, 2024

cshannon deleted the accumulo-4183 branch March 15, 2024 11:09

ctubbsii added this to the 4.0.0 milestone Jul 12, 2024

	txStore = reservedTxStore;
	Precondition.checkState(fateId.equals(fateIdFromCreate));
	txStore = reservedTxStore;

Add support for creating a FATE transaction by key #4204

Add support for creating a FATE transaction by key #4204

Uh oh!

Conversation

cshannon commented Jan 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

keith-turner commented Jan 29, 2024

Uh oh!

cshannon commented Jan 30, 2024

Uh oh!

Uh oh!

Uh oh!

cshannon commented Feb 4, 2024

Uh oh!

keith-turner left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

keith-turner Feb 7, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

keith-turner Feb 7, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

keith-turner Feb 7, 2024

Choose a reason for hiding this comment

Uh oh!

cshannon Feb 9, 2024

Choose a reason for hiding this comment

Uh oh!

keith-turner commented Feb 8, 2024

Uh oh!

cshannon commented Feb 8, 2024

Uh oh!

keith-turner commented Feb 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cshannon commented Feb 9, 2024

Uh oh!

keith-turner Feb 9, 2024

Choose a reason for hiding this comment

Uh oh!

keith-turner Feb 9, 2024

Choose a reason for hiding this comment

Uh oh!

keith-turner Feb 9, 2024

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

cshannon commented Jan 27, 2024 •

edited

Loading

keith-turner commented Feb 8, 2024 •

edited

Loading