Makes seeding a fate transaction more efficient #5122

keith-turner · 2024-11-28T00:54:08Z

Modified fate to seed fate transaction in single conditional mutation instead of multiple.

Modified fate to seed fate transaction in single conditional mutation instead of multiple. fixes apache#5097

kevinrr888

Most of my comments are trivial or just observations. Logic looks good and consistent with previous impl with the added improvement of a single condition mutation for seeding a USER transaction. Some small questions/potential concerns. Verified fate tests still pass. Also, I have not looked through all of the FateStoreIT changes yet.

core/src/main/java/org/apache/accumulo/core/fate/user/FateMutator.java

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

core/src/main/java/org/apache/accumulo/core/fate/zookeeper/MetaFateStore.java

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

kevinrr888 · 2024-12-05T19:17:02Z

core/src/main/java/org/apache/accumulo/core/fate/user/UserFateStore.java

+        // signal to the super class that a new fate transaction was seeded and is ready to run
+        seededTx();
+        return true;
+      } else if (status == FateMutator.Status.REJECTED) {


I'm wondering about the following case:
The first attempt is actually written but returns a status of UNKNOWN. Then, any subsequent attempts will return REJECTED causing seedTransaction to return that it failed/false.
This was something that was previously handled

Good point. Looking over the way things works end to end if a lot of effort is made here to determine what happened with unknown then the calling code will not really care (because it just logs the result). However the way the functions are structured that is not obvious and if future calling code did something besides just logging the result then it would not be correct in this corner case. I will try to look at the end to end structure and clean this up to either handle that corner case or make the functions reflect a best effort impl.

Looking at this some more I will make it try to determine what happened. Even thought its just logging, having accurate log message is important for debugging. And I like the info provided by the current log messages, so do not want to remove them.

Tried to handle the unknown case and realized its impossible to determine what happened in this case. Prior to these changes a reservation was acquired and that has unique uuid which can be used to determine if the reservation was acquired when unknown is seen. Now that this code is using a single conditional mutation, no reservation is required. The reservation is only required for updates that happen over multiple conditional mutations.

So since there is no way to know what happened, I adjusted the logging and code to reflect this in 89b70da

Ah, yeah you're right...
The new comment for the UNKNOWN case looks good. The new mention of "possibly unable to seed" in FateLogger looks good.

core/src/main/java/org/apache/accumulo/core/fate/user/UserFateStore.java

core/src/main/java/org/apache/accumulo/core/fate/zookeeper/MetaFateStore.java

kevinrr888 · 2024-12-05T19:59:28Z

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

+   * Seeds a transaction with the given repo if it does not exists. Will set the status to
+   * SUBMITTED. Will set the fate key. Will set autoCleanup if true. Will set the creation time.
   *
-   * <p>
-   * In the case where a process dies in the middle of a call to this. If later, another call is
-   * made with the same key and its in the new state then the FateId for that key will be returned.
-   * </p>
+   * @return optional w/ the fate id set if seeded and empty optional otherwise
+   */
+  Optional<FateId> seedTransaction(String txName, FateKey fateKey, Repo<T> repo,
+      boolean autoCleanUp);
+
+  /**
+   * Seeds a transaction with the given repo if its current status is NEW and it is currently
+   * unreserved. Will set the status to SUBMITTED. Will set autoCleanup if true. Will set the
+   * creation time.
   *
-   * @throws IllegalStateException when there is an unexpected collision. This can occur if two key
-   *         hash to the same FateId or if a random FateId already exists.
+   * @return true if seeded and false otherwise
   */
-  Optional<FateTxStore<T>> createAndReserve(FateKey fateKey);
+  boolean seedTransaction(String txName, FateId fateId, Repo<T> repo, boolean autoCleanUp);


Should also add that it will set the txName to these descriptions.

updated in 7110fd7

Looks good. I also like the new @ return description for these methods indicating result may not be entirely accurate in case of failure

core/src/main/java/org/apache/accumulo/core/fate/zookeeper/MetaFateStore.java

…tor.java Co-authored-by: Kevin Rathbun <kevinrr888@gmail.com>

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

kevinrr888 · 2024-12-09T22:31:10Z

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

+    txStore = store.reserve(fateId1);
+    try {
+      assertEquals(TStatus.IN_PROGRESS, txStore.getStatus());
+      txStore.forceDelete();


could also check top() here before deleting

kevinrr888 · 2024-12-09T22:37:29Z

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

-    txStore.setTransactionInfo(TxInfo.TX_NAME, txName);
-
-    txStore.setStatus(SUBMITTED);
+    return store.seedTransaction(txName, fateKey, repo, autoCleanUp);


I agree that FateKey contains the same info. Those changes look good.

kevinrr888 · 2024-12-09T22:39:30Z

core/src/main/java/org/apache/accumulo/core/fate/user/UserFateStore.java

+        // signal to the super class that a new fate transaction was seeded and is ready to run
+        seededTx();
+        return true;
+      } else if (status == FateMutator.Status.REJECTED) {


Ah, yeah you're right...
The new comment for the UNKNOWN case looks good. The new mention of "possibly unable to seed" in FateLogger looks good.

kevinrr888

These changes look good to me. I also looked through the FateStoreIT changes and those look good. I like the new testCreate and testConcurrent tests.

Co-authored-by: Kevin Rathbun <kevinrr888@gmail.com>

Makes seeding a fate transaction more efficient

760dbff

Modified fate to seed fate transaction in single conditional mutation instead of multiple. fixes apache#5097

keith-turner added this to the 4.0.0 milestone Nov 28, 2024

keith-turner requested a review from kevinrr888 December 5, 2024 16:42

kevinrr888 reviewed Dec 5, 2024

View reviewed changes

keith-turner and others added 5 commits December 5, 2024 18:54

Update core/src/main/java/org/apache/accumulo/core/fate/user/FateMuta…

4ecfb87

…tor.java Co-authored-by: Kevin Rathbun <kevinrr888@gmail.com>

code review updates

e47089c

code review updates

7110fd7

code review update

89b70da

improve log message

42f8b4d

kevinrr888 reviewed Dec 9, 2024

View reviewed changes

kevinrr888 approved these changes Dec 9, 2024

View reviewed changes

keith-turner and others added 2 commits December 9, 2024 18:41

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

3b2d9c8

Co-authored-by: Kevin Rathbun <kevinrr888@gmail.com>

code review update

51ad654

keith-turner mentioned this pull request Dec 10, 2024

Improve performance of seeding split fate operations #5160

Open

keith-turner merged commit 6407c31 into apache:main Dec 10, 2024
8 checks passed

keith-turner deleted the speed-up-seeding branch December 10, 2024 00:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Makes seeding a fate transaction more efficient #5122

Makes seeding a fate transaction more efficient #5122

keith-turner commented Nov 28, 2024

kevinrr888 left a comment •

edited

Loading

kevinrr888 Dec 5, 2024

keith-turner Dec 5, 2024

keith-turner Dec 6, 2024 •

edited

Loading

keith-turner Dec 6, 2024

kevinrr888 Dec 9, 2024

kevinrr888 Dec 5, 2024

keith-turner Dec 6, 2024

kevinrr888 Dec 9, 2024 •

edited

Loading

kevinrr888 Dec 9, 2024

kevinrr888 Dec 9, 2024

kevinrr888 Dec 9, 2024

kevinrr888 left a comment

Makes seeding a fate transaction more efficient #5122

Makes seeding a fate transaction more efficient #5122

Conversation

keith-turner commented Nov 28, 2024

kevinrr888 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

keith-turner Dec 6, 2024 • edited Loading

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

kevinrr888 Dec 9, 2024 • edited Loading

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

kevinrr888 left a comment

Choose a reason for hiding this comment

kevinrr888 left a comment •

edited

Loading

keith-turner Dec 6, 2024 •

edited

Loading

kevinrr888 Dec 9, 2024 •

edited

Loading