Add getTxStatus RPC #1111
Add getTxStatus RPC #1111
Conversation
|
tx_status@7906 aka 20200501.10 vs master ewma over 50 builds from 7390 to 7895 |
|
I suggest we start with #968 with new endpoints, to minimise future disruption to users. Not sure about LOST, I think ROLLED_BACK is more precise. I believe we'd agreed a |
I was thinking ROLLED_BACK is too Raft-specific, its not really true for PBFT or a general consensus? We don't know if your transaction was partially accepted then rolled back (and the fact we do this in the kv seems like an implementation detail - in standard Raft terminology these 'local commits' are just stored in a pending lost, not applied then rolled back). The general externally visible behaviour is just "this didn't happen". And we've settled on |
|
@eddyashton there's a tradeoff between consistency and user-pain here. If we start making the new endpoints from this one onwards correct, users who rely on those won't have to update the code that deals with it. If we go for consistency, we're making more work for ourselves and for users, in return for slightly less surprise across RPCs. I would prefer to go straight for the end state here, and to add an OpenAPI schema for this, but I'd happy to hear everyone's opinion about this too. Lost to me sounds like Rolled back but worse, it's an acknowledgement it existed, but then it was misplaced. I do agree that rolled back isn't quite right either. |
|
How about: |
|
|
|
|
|
||
| This example queries the status of transaction ID 2.18 (constructed from view 2 and sequence number 18). The response indicates this was successfully committed. The headers also show that the service has since made progress with other requests, and global commit index has continued to increase. | ||
|
|
||
| The possible statuses are: | ||
|
|
There was a problem hiding this comment.
I know we are not removing all the mentions to globally committed just yet, but I think that in the description of the statuses it is a bit confusing. I would say globally replicated, replicated to enough nodes to make the transaction durable, or something similar
|
|
||
| It is possible that intermediate states are not visible (eg - a transition from Unknown to Committed may never publically show a Pending result). Nodes may disagree on the current state due to communication delays, but will never disagree on transitions (in other words, they may believe a Committed transaction is still Unknown or Pending, but will never report it as Invalid). | ||
|
|
||
| Note that transaction IDs are uniquely assigned by the service - once a request has been assigned an ID, this ID will never be associated with a different write transaction. In normal operation, the next requests will be given versions 2.19, then 2.20, and so on, and after a short delay 2.18 will be globally committed. If requests are submitted in parallel, they will be applied in a consistent order indicated by their assigned versions. If the network is unable to reach consensus, it will trigger a leadership election which increments the view. In this case the user's next request may be given a version 3.16, followed by 3.17, then 3.18. The sequence number is reused, but in a different view; the service knows that 2.18 can never be assigned, so it can report this as an invalid ID. Read-only transactions are an exception - they do not get a unique transaction ID but instead return the ID of the last write transaction whose state they may have read. |
There was a problem hiding this comment.
I'd suggest to remove globally committed from this paragraph too and just leave it as committed
| @@ -251,49 +253,36 @@ namespace timing | |||
| record_receive(response.id, commit_ids); | |||
| } | |||
|
|
|||
| const auto response_term = body["term"].get<size_t>(); | |||
| if (response_term == 0) | |||
| // NB: Eventual header re-org should be exposing API types so | |||
There was a problem hiding this comment.
That’s... a submarine TODO? I would suggest updating the headers ticket instead.
| x-ccf-global-commit: 33 | ||
| x-ccf-term: 2 | ||
| x-ccf-commit: 42 | ||
| x-ccf-global-commit: 40 |
There was a problem hiding this comment.
I thought we were changing those to seqno/view? Next time then.
This adds a
getTxStatusRPC, replacing thegetCommit(N)variant.getCommitnow has a single purpose of fetching the current kv version, and the "is this committed" logic is handled in a single place, by the service.I'm unsure about naming:
termandcommitfor consistency with existing RPCs/header names/externally facing things, are we planning to convert all of these toviewandindex? FWIW, I'm still fond of formalisingversion = view.index, so I've sprinkled that into the docs.LOST, but am open to suggestions.UNKNOWN(I've never heard of this transaction) state distinct fromPENDING(I know it, but I don't know if it is globally committed)? A long time in theUNKNOWNstate may indicate that we've had an election but haven't yet reached your index, but this is only a heuristic (not true for partitioned nodes), and introduces possibility of odd response chains (UNKNOWN->PENDING->UNKNOWN->PENDING-> ... -> [COMMITTED|LOST]).I believe in the current implementation, if you ask a single node, the response will always transition exactly once from
PENDINGto eitherCOMMITTEDorLOST, and will be that way forever. And the transition will be consistent across all nodes - once you've heardCOMMITTED, another node may still sayPENDING, but can never sayLOST(and vice versa). These are things I don't think we've clearly described in the docs, so I've added a note - let me know if it makes sense.I've also removed the
SigHttpRpcTlsClient- its functionality was almost entirely (and now entirely) handled by the baseHttpRpcTlsClient.EDIT - leaving the original description for posterity, but here's a more accurate current state:
/tx(ie/users/tx,/members/tx), as an exemplar of where we want to go with HTTP naming styleview.seqno. Although we currently usetermandcommitin response headers/other RPCs, we intend to rename soon.UNKNOWN,PENDING,COMMITTED, andINVALID. Unknown and Pending can be usefully distinguished, and we can report Invalid in some cases I'd missed initially. There's a non-systematic unit test for how we derive this status.