Globally Unique FATE Transaction Ids - Part 1 #4191

kevinrr888 · 2024-01-24T20:51:34Z

PR for #4044. The end goal is to have the stronger type FateId replace the current representation of a transaction id (which is just a long). This was brought about from the addition of the AccumuloStore class - there are now two fate instance types associated with a transaction - META (for ZooStore) or USER (for AccumuloStore). FateId is a new class which includes the FateInstanceType and the transaction id.

TODO:

Create new class FateId to replace FateTxId
Change the stores to use FateId. Start with ReadOnlyFateStores. Resolve issues stemming from these changes.
Change Fate to use FateId. Resolve issues stemming from these changes.
Change Repo to use FateId. Resolve issues stemming from these changes.
AdminUtil and Admin need to use FateId. Deferred for now. Need #4168 completed first.
Delete FateTxId when all uses have been replaced with FateId

This PR is ready for review and potentially to be merged. Will need at least another PR for the rest of the changes.

A WIP for apache#4044. The end goal is to have the stronger type FateId replace the current representation of a transaction id (which is just a long). This was brought about from the addition of the AccumuloStore class - there are now two fate instance types associated with a transaction - META (for ZooStore) or USER (for AccumuloStore). FateId is a new class which includes the FateInstanceType and the transaction id. Current changes: - FateTxId replaced with new class FateId - Started with changes in ReadOnlyFateStore and resolved the issues in other classes extending from changing this class. Still left TODO: - There are still related problems from the classes I have changed: I have not yet finished going down the entire chain of issues - Need to change Fate and the associated issues with changing this class - Need to change Repo and the associated issues with changing this class - More changes TBD

keith-turner · 2024-01-24T21:07:02Z

In ReadOnlyFateStore will need to change the following to

void runnable(AtomicBoolean keepWaiting, LongConsumer idConsumer);

void runnable(AtomicBoolean keepWaiting, Consumer<FateId> idConsumer);

keith-turner

These changes look good so far. Based on what you have done so far do you think it would be possible to break this work in multiple PRs?

keith-turner · 2024-01-24T21:16:17Z

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

@@ -82,7 +85,9 @@ protected Stream<FateIdStatus> getTransactions() {
      scanner.setRange(new Range());
      TxColumnFamily.STATUS_COLUMN.fetch(scanner);
      return scanner.stream().onClose(scanner::close).map(e -> {
-        return new FateIdStatus(parseTid(e.getKey().getRow().toString())) {
+        FateId fateId =
+            FateId.from("FATE:" + fateInstanceType + ":" + e.getKey().getRow().toString());


Could have a FateId.from(FateInstanceType it, String hexId) method and do the following

Suggested change

FateId.from("FATE:" + fateInstanceType + ":" + e.getKey().getRow().toString());

FateId.from(fateInstanceType, e.getKey().getRow().toString());

Yes, good suggestion

kevinrr888 · 2024-01-24T21:38:46Z

In ReadOnlyFateStore will need to change the following to

void runnable(AtomicBoolean keepWaiting, LongConsumer idConsumer);

void runnable(AtomicBoolean keepWaiting, Consumer<FateId> idConsumer);

Ah okay. Yeah wasn't too sure exactly how I should go about changing this method. Will make suggested change.

kevinrr888 · 2024-01-24T21:57:17Z

These changes look good so far. Based on what you have done so far do you think it would be possible to break this work in multiple PRs?

Yes, this is my thinking of how it could be broken up:

A PR changing ReadOnlyFateStore.java and the issues stemming from this (current PR)
A PR resolving all the issues from deleting FateTxId.java (maybe combined with first)
A PR changing Fate.java and the issues stemming from this
A PR changing Repo.java and the issues stemming from this
Potentially more PRs

However, I'm not sure if this is the best way or if these PRs standalone would actually pass all the git checks/not break the project. Not sure how interlinked everything is.

Will be deleted later on when it can be safely deleted (after all uses of FateTxId have been replaced by FateId)

cshannon · 2024-01-26T11:52:03Z

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

+   * @return a new FateId object
+   */
+  public static FateId from(FateInstanceType type, String hexTid) {
+    Pattern hexPattern = Pattern.compile("^[0-9a-fA-F]+$");


This can be moved to a static private field so that we can cache the compiled pattern and avoid the expense of creating it each time. Java patterns are thread

- New fromThrift() method in FateId. - Fate now uses FateId and resolved issues stemming from these changes. - AdminUtil and Admin do not have FateId fully incorporated yet. Deferred for now. Marked "TODO DEFERRED - ISSUE 4044" in code. Need issue apache#4168 completed first. - Resolved all compile errors.

kevinrr888 · 2024-01-30T15:32:38Z

Update:

New fromThrift() method in FateId.
Fate now uses FateId and resolved issues stemming from these changes.
AdminUtil and Admin do not have FateId fully incorporated yet. Deferred for now. Marked "TODO DEFERRED - ISSUE 4044" in code. Need #4168 completed first.
Resolved all compile errors.
Now that it compiles, I am able to run tests but have found that several hang indefinitely. Need to resolve this. (edit: Resolved)
Updated the main PR comment to better break down overall goal into smaller sub goals.

keith-turner

These changes look good

keith-turner · 2024-01-30T22:58:05Z

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


          String txName = (String) txStore.getTransactionInfo(Fate.TxInfo.TX_NAME);

-          List<String> hlocks = heldLocks.remove(tid);
+          // TODO DEFERRED - ISSUE 4044


We have been using a more specific TODO in the elasticity branch, could use it here

Suggested change

// TODO DEFERRED - ISSUE 4044

// ELASTICITY_TODO DEFERRED - ISSUE 4044

keith-turner · 2024-01-30T23:01:45Z

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

@@ -237,8 +236,8 @@ private void blockIfHadoopShutdown(long tid, Exception e) {
    }

    private void transitionToFailed(FateTxStore<T> txStore, Exception e) {
-      String tidStr = FateTxId.formatTid(txStore.getID());
-      final String msg = "Failed to execute Repo " + tidStr;
+      FateId fateId = txStore.getID();


Could inline this, it was only there to avoid duplicating the FateTxId.formatTid(...) code.

- Inline statement in Fate - Changed comment about deferral in AdminUtil

keith-turner reviewed Jan 24, 2024

View reviewed changes

kevinrr888 changed the title ~~Globally Unique FATE Transaction Ids~~ WIP Globally Unique FATE Transaction Ids Jan 25, 2024

kevinrr888 added 3 commits January 25, 2024 10:41

runnable() sig changed, FateId.from() sig changed

25e17ab

Added back FateTxId.java

117149d

Will be deleted later on when it can be safely deleted (after all uses of FateTxId have been replaced by FateId)

Merge branch 'elasticity' into elasticity-feature-4044

5b059d5

cshannon reviewed Jan 26, 2024

View reviewed changes

pattern to private static final field

704fd68

cshannon mentioned this pull request Jan 27, 2024

Add support for creating a FATE transaction by key #4204

Merged

Fixed Test Failures

2e7cf34

keith-turner reviewed Jan 31, 2024

View reviewed changes

kevinrr888 added 2 commits January 31, 2024 11:12

Merge branch 'elasticity' into elasticity-feature-4044

8d48a3a

Added requested changes:

9e4710c

- Inline statement in Fate - Changed comment about deferral in AdminUtil

kevinrr888 marked this pull request as ready for review January 31, 2024 17:08

kevinrr888 changed the title ~~WIP Globally Unique FATE Transaction Ids~~ Globally Unique FATE Transaction Ids - Part 1 Jan 31, 2024

Trivial change to AccumuloStore.create()

45eeaed

keith-turner approved these changes Feb 1, 2024

View reviewed changes

keith-turner merged commit 1d51e46 into apache:elasticity Feb 1, 2024
8 checks passed

This was referenced Feb 1, 2024

Update FATE transaction ids to be globally unique across multiple stores #4044

Closed

Globally Unique FATE Transaction Ids - Part 2 #4228

Merged

This was referenced Feb 16, 2024

Admin and AdminUtil should use FateId #4274

Closed

Remove FateTxId in Elasticity #4275

Closed

FateId's tid should be replaced with 128bit tid #4277

Closed

ctubbsii added this to the 4.0.0 milestone Jul 12, 2024

kevinrr888 deleted the elasticity-feature-4044 branch November 1, 2024 15:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Globally Unique FATE Transaction Ids - Part 1 #4191

Globally Unique FATE Transaction Ids - Part 1 #4191

kevinrr888 commented Jan 24, 2024 •

edited

Loading

keith-turner commented Jan 24, 2024

keith-turner left a comment

keith-turner Jan 24, 2024

kevinrr888 Jan 24, 2024

kevinrr888 commented Jan 24, 2024

kevinrr888 commented Jan 24, 2024

cshannon Jan 26, 2024

kevinrr888 commented Jan 30, 2024 •

edited

Loading

keith-turner left a comment

keith-turner Jan 30, 2024

keith-turner Jan 30, 2024

	FateId.from("FATE:" + fateInstanceType + ":" + e.getKey().getRow().toString());
	FateId.from(fateInstanceType, e.getKey().getRow().toString());

	// TODO DEFERRED - ISSUE 4044
	// ELASTICITY_TODO DEFERRED - ISSUE 4044

Globally Unique FATE Transaction Ids - Part 1 #4191

Globally Unique FATE Transaction Ids - Part 1 #4191

Conversation

kevinrr888 commented Jan 24, 2024 • edited Loading

keith-turner commented Jan 24, 2024

keith-turner left a comment

Choose a reason for hiding this comment

keith-turner Jan 24, 2024

Choose a reason for hiding this comment

kevinrr888 Jan 24, 2024

Choose a reason for hiding this comment

kevinrr888 commented Jan 24, 2024

kevinrr888 commented Jan 24, 2024

cshannon Jan 26, 2024

Choose a reason for hiding this comment

kevinrr888 commented Jan 30, 2024 • edited Loading

keith-turner left a comment

Choose a reason for hiding this comment

keith-turner Jan 30, 2024

Choose a reason for hiding this comment

keith-turner Jan 30, 2024

Choose a reason for hiding this comment

kevinrr888 commented Jan 24, 2024 •

edited

Loading

kevinrr888 commented Jan 30, 2024 •

edited

Loading