Added FaTE TStatus.SUBMITTED by dlmarion · Pull Request #2462 · apache/accumulo

dlmarion · 2022-02-03T19:11:55Z

When a FaTE transaction is created in the Manager its status is set
to IN_PROGRESS even though it's not actually running, it could be
waiting to run. The output from the fate print command will show
that the newly created transaction is IN_PROGRESS and could be
confusing to users. This change introduces the SUBMITTED state, which
is the state of the transaction after it has been created but before
it is executed.

When a FaTE transaction is created in the Manager its status is set to IN_PROGRESS even though it's not actually running, it could be waiting to run. The output from the `fate print` command will show that the newly created transaction is IN_PROGRESS and could be confusing to users. This change introduces the SUBMITTED state, which is the state of the transaction after it has been created but before it is executed.

ctubbsii

I'm not really sure I understand the difference between the NEW state that already exists and your new SUBMITTED state.

Co-authored-by: Christopher Tubbs <ctubbsii@apache.org>

dlmarion · 2022-02-03T20:09:58Z

I'm not really sure I understand the difference between the NEW state that already exists and your new SUBMITTED state.

Prior to this change, when Fate.startTransaction was called to allocate a transaction id its state would be NEW. Then, when Fate.seedTransaction() was called to associate the Repo with the transaction id, the state would be changed to IN_PROGRESS. Fate.startTransaction would just serialize the Repo into ZooKeeper, it didn't actually run it.

With this change, there is an intermediate state (SUBMITTED).

Here's an excerpt from FateIT:

      long txid = fate.startTransaction();
      assertEquals(TStatus.NEW, getTxStatus(zk, txid));
      fate.seedTransaction(txid, new TestOperation(NS, TID), true);
      assertEquals(TStatus.SUBMITTED, getTxStatus(zk, txid));

EdColeman · 2022-02-03T21:25:50Z

I took a quick look and confirmed that the metrics look okay - the TX state is added to the expected states.

…eIT.java

No requested changes. Just a comment remaining.

…D state

milleruntime · 2022-02-04T14:28:19Z

I just had a thought for an alternative approach. I am not sure if it is a better solution to adding the SUBMITTED state though. Would it be easier to change the code to set the state to NEW when the transaction is seeded? We know the transaction is new because it will have an ID so I am just wondering if we need an additional state.

dlmarion · 2022-02-04T14:35:14Z

So, currently the NEW fate transactions are aged off. If we moved NEW to mean SUBMITTED, then we would need another state to still exist so that they could be aged off. At that point, we're at the same place.

milleruntime · 2022-02-04T14:43:43Z

So, currently the NEW fate transactions are aged off. If we moved NEW to mean SUBMITTED, then we would need another state to still exist so that they could be aged off. At that point, we're at the same place.

I was thinking to not store the TxD in ZK. It doesn't look like we are really doing anything between the creation and seeding.

dlmarion · 2022-02-04T14:46:31Z

I was thinking to not store the TxD in ZK. It doesn't look like we are really doing anything between the creation and seeding.

I'm not sure of the side effects of that approach, if any. Will need someone else with more FaTE knowledge to chime in.

milleruntime · 2022-02-04T15:03:49Z

I was thinking to not store the TxD in ZK. It doesn't look like we are really doing anything between the creation and seeding.

I'm not sure of the side effects of that approach, if any. Will need someone else with more FaTE knowledge to chime in.

Yeah, I am not sure either. One side effect, which I think would be a good one, would be less TxIDs created in ZK. Currently the call to create the TxID is technically repeatable but it will create a lot of extra TxIDs in ZK and extra calls to ZK to age them off. The TxID is just a random number so its not like we are tracking it for other purposes, like we do with TableId.

ctubbsii · 2022-02-04T16:10:33Z

The TxID is just a random number so its not like we are tracking it for other purposes, like we do with TableId.

Isn't storing it in ZK guaranteeing it's unique, though?

milleruntime · 2022-02-04T16:27:55Z

The TxID is just a random number so its not like we are tracking it for other purposes, like we do with TableId.

Isn't storing it in ZK guaranteeing it's unique, though?

Yes

dlmarion · 2022-02-04T17:15:50Z

@milleruntime - are you good with this approach then considering the discussion?

milleruntime · 2022-02-04T17:21:20Z

@milleruntime - are you good with this approach then considering the discussion?

Yeah I think this approach is fine. I wanted to look through the code closer but if its holding you up, feel free to merge.

milleruntime · 2022-02-04T17:43:17Z

        store.setProperty(tid, DEBUG_PROP, repo.getDescription());

-        store.setStatus(tid, TStatus.IN_PROGRESS);
+        store.setStatus(tid, TStatus.SUBMITTED);


I think there are some OPs that do work in the isReady() function so I wonder if changing when it gets set to IN_PROGRESS will affect them. I will see if I can find one.

The BulkImport and Compact operations do things in the isReady() function. I don't know if it matters if the status is IN_PROGRESS or SUBMITTED though.

Also the ShutdownTServer operation

milleruntime

As far as I can tell, moving the setting of the status, shouldn't affect the business logic in the operations. But we should probably do something about the operations that are doing stuff in the isReady() method since we can't assume this method is just checking things. It looks like the worst offenders are: BulkImport, Compact and the ShutdownTServer operations

milleruntime · 2022-02-04T18:02:48Z

I created a follow on: #2466

dlmarion self-assigned this Feb 3, 2022

ctubbsii requested changes Feb 3, 2022

View reviewed changes

Compare TStatus enums with ==

bacb2c3

Co-authored-by: Christopher Tubbs <ctubbsii@apache.org>

Changes from review

46a503a

dlmarion requested a review from ctubbsii February 3, 2022 20:38

ctubbsii previously requested changes Feb 3, 2022

View reviewed changes

Comment thread core/src/main/java/org/apache/accumulo/fate/AgeOffStore.java Outdated

Comment thread test/src/main/java/org/apache/accumulo/test/fate/zookeeper/FateIT.java Outdated

Update test/src/main/java/org/apache/accumulo/test/fate/zookeeper/Fat…

a8a1c11

…eIT.java

Modified AgeOffStore to not age off Fate transactions in the SUBMITTE…

f228aab

…D state

dlmarion mentioned this pull request Feb 4, 2022

Add parameter to Merge command to cancel blocking fate operations #2328

Closed

ctubbsii approved these changes Feb 4, 2022

View reviewed changes

milleruntime reviewed Feb 4, 2022

View reviewed changes

milleruntime approved these changes Feb 4, 2022

View reviewed changes

dlmarion merged commit a80cf36 into apache:main Feb 4, 2022

dlmarion deleted the fate_submitted_status branch February 4, 2022 18:13

dlmarion mentioned this pull request Feb 7, 2022

Added mechanism to cancel FaTE operations from Shell with Manager up #2467

Merged

ctubbsii mentioned this pull request Feb 9, 2022

Broken or Flaky test: FateIT #2474

Closed

ctubbsii added this to the 2.1.0 milestone Jul 12, 2024

Uh oh!

Conversation

dlmarion commented Feb 3, 2022

Uh oh!

ctubbsii left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dlmarion commented Feb 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

EdColeman commented Feb 3, 2022

Uh oh!

Uh oh!

Uh oh!

milleruntime commented Feb 4, 2022

Uh oh!

dlmarion commented Feb 4, 2022

Uh oh!

milleruntime commented Feb 4, 2022

Uh oh!

dlmarion commented Feb 4, 2022

Uh oh!

milleruntime commented Feb 4, 2022

Uh oh!

ctubbsii commented Feb 4, 2022

Uh oh!

milleruntime commented Feb 4, 2022

Uh oh!

dlmarion commented Feb 4, 2022

Uh oh!

milleruntime commented Feb 4, 2022

Uh oh!

milleruntime Feb 4, 2022

Choose a reason for hiding this comment

Uh oh!

milleruntime Feb 4, 2022

Choose a reason for hiding this comment

Uh oh!

milleruntime Feb 4, 2022

Choose a reason for hiding this comment

Uh oh!

milleruntime left a comment

Choose a reason for hiding this comment

Uh oh!

milleruntime commented Feb 4, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

dlmarion commented Feb 3, 2022 •

edited

Loading