addressed review comments

Signed-off-by: Harshit Gangal <[email protected]>

doc/design-docs/AtomicDistributedTransaction.md

@@ -31,7 +31,7 @@ Vitess will add a few variations to the traditional 2PC algorithm:
 * The 2PC algorithm does not specify how the Transaction Manager maintains the metadata. If you work through all the failure modes, it becomes evident that the manager must also be a highly available (HA) transactional system that survives failures without data loss. Since the VTTablets are already built to be HA, there’s no reason to build yet another system. So, we will split the role of the Transaction Manager into two:
   -   The Coordinator will be stateless and will orchestrate the work. VTGates are the perfect fit for this role.
   -   One of the VTTablets will be designated as the Metadata Manager (MM). It will store the metadata and perform the necessary state transitions.
-* If we designate one of the participant VTTablets to be the MM, that database can avoid the prepare phase. If you assume there are N participants, the typical explanation says to perform prepares from 1->N, and then commit from 1->N. Instead, we could go from 1->N for prepare, and N->1 for commit. Then, the Nth database would perform a Prepare->Decide to Commit->Commit. Instead, we execute the DML needed to transition the metadata state to "Decide to Commit" as part of the app transaction and commit it. If the commit fails, it is treated as the prepare having failed. If the commit succeeds, it is treated as all three operations having succeeded.
+* If we designate one of the participant VTTablets to be the MM, that database can avoid the prepare phase. If you assume there are N participants, the typical process is to perform prepares from 1 to N, followed by commits from 1 to N. Instead, we could go from 1->N for prepare, and N->1 for commit. Then, the Nth database would perform a Prepare->Decide to Commit->Commit. Instead, we execute the DML needed to transition the metadata state to "Decide to Commit" as part of the app transaction and commit it. If the commit fails, it is treated as the prepare having failed. If the commit succeeds, it is treated as all three operations having succeeded.
 * The Prepare functionality will be implemented as explained in the [blog](https://vitess.io/blog/2016-06-07-distributed-transactions-in-vitess/).
 
 Combining the above changes allows us to keep the most common use case efficient. A transaction that affects only one database incurs no additional cost due to 2PC.
@@ -208,7 +208,7 @@ sequenceDiagram
   G ->>- App: Err Packet
 ```
 
-Case 3: When the Commit Descision from MM responds with an error. In this case, the watcher service needs to resolve the transaction as it is not certain whether the commit decision persisted or not.
+Case 3: When the Commit Decision from MM responds with an error. In this case, the watcher service needs to resolve the transaction as it is not certain whether the commit decision persisted or not.
 ```mermaid
 sequenceDiagram
   participant App as App
@@ -334,8 +334,8 @@ This is needed mainly so that the watchdog process can pick up an orphaned trans
 
 The DTID will be generated by taking the VTID of the MM and prefixing it with the keyspace and shard info to prevent collisions. 
 If the MM’s VTID is ‘1234’ for keyspace ‘order’ and shard ‘40-80’, then the DTID will be ‘order:40-80:1234’. 
-A collision could still happen if there is a failover and the new vttablet’s starting VTID had overlaps with the previous instance. 
-To prevent this, the starting VTID of the vttablet will be adjusted to a value higher than any used by the prepared DTIDs.
+A collision could still happen if there is a failover and the new VTTablet’s starting VTID had overlaps with the previous instance. 
+To prevent this, the starting VTID of the VTTablet will be adjusted to a value higher than any used by the prepared DTIDs.
 
 ## Prepare API
 
@@ -391,10 +391,10 @@ This function is proposed to take a DTID and a VTID as input.
 
 If VTTablet is being shut down or transitioned to a non-primary, the transaction pool handler will internally, rollback the prepared transactions and return them to the transaction pool. 
 The rollback of prepared transactions must happen only after all the open transactions are resolved (rollback or commited). 
-If a pending transaction is waiting on a lock held by a prepared transaction, it will eventually timeout and get rolled back.
+If a pending transaction is waiting on a lock held by a prepared transaction, it will eventually time out and get rolled back.
 
 Eventually, a different VTTablet will be transitioned to become the primary. At that point, it will recreate the unresolved transactions from redo logs. 
-If the replays fail, we’ll raise an alert and start the query service anyway. Typically, a replay is not expected to fail because vttablet does not allow writing to the database until the replays are done. Also, no external agent should be allowed to perform writes to MySQL, which is a loosely enforced Vitess requirement. Other vitess processes do write to MySQL directly, but they’re not the kind that interfere with the normal flow of transactions.
+If the replays fail, we’ll raise an alert and start the query service anyway. Typically, a replay is not expected to fail because VTTablet does not allow writing to the database until the replays are done. Also, no external agent should be allowed to perform writes to MySQL, which is a loosely enforced Vitess requirement. Other vitess processes do write to MySQL directly, but they’re not the kind that interfere with the normal flow of transactions.
 
 VTTablet always execute DMLs with BEGIN-COMMIT. This will ensure that no autocommit statements can slip through if connections are inadvertently closed out of sequence.
 
@@ -475,7 +475,7 @@ If not successful, VTGate at this point will leave the transaction resolution to
 
 This function transitions the state from PREPARE to ROLLBACK using an independent transaction. 
 When this function is called, the MM’s transaction (VTID) may still be alive. 
-So, it infers the transaction id from the dtid and perform a best effort rollback. 
+So, it infers the transaction id from the dtid and perform the best effort rollback. 
 If the transaction is not found, it’s a no-op.
 
 ### ConcludeTransaction
@@ -499,7 +499,7 @@ This function returns all transaction metadata and the redo statement log.
 VTGate is already responsible for Best Effort Commit, aka `transaction_mode=MULTI`, it can naturally be extended to act as the coordinator for 2PC. 
 It needs to support commit with `transaction_mode=twopc`.
 
-VTGate also have to listen on VTTablet healthstream to receive unresolved transaction signal and act on it to resolve them.
+VTGate also has to listen on the VTTablet health stream to receive unresolved transaction signals and act on them to resolve them.
 
 ### Commit(transaction_mode=twopc)
 
@@ -543,7 +543,7 @@ Rollback workflow:
 ## Transaction Resolution Watcher
 
 The stateless VTGates are considered ephemeral and can fail at any time, which means that transactions could be abandoned in the middle of a distributed commit. 
-To mitigate this, every primary vttablet will poll its dt_state table for distributed transactions that are lingering. 
+To mitigate this, every primary VTTablet will poll its dt_state table for distributed transactions that are lingering. 
 If any such transaction is found, it will signal this to VTGate via health stream to resolve them.
 
 ## Client API
@@ -570,17 +570,17 @@ The above outlined steps ensure that we either wait for all prepared transaction
 
 On the new primary, when we call `PromoteReplica`, we redo all the prepared transactions before we allow any new writes to go through. This ensures that the new primary is in the same state as the old primary was before the reparent. The code for redoing the prepared transactions can be found in `TxEngine.RedoPreparedTransactions()`.
 
-If everything goes as described above, there is no reason for redoing of prepared transactions to fail. But in case, something unexpected happens and preparing transactions fails, we still allow the vttablet to accept new writes because we decided availability of the tablet is more important. We will however, build tooling and metrics for the users to be notified of these failures and let them handle this in the way they see fit.
+If everything goes as described above, there is no reason for redoing of prepared transactions to fail. But in case, something unexpected happens and preparing transactions fails, we still allow the VTTablet to accept new writes because we decided availability of the tablet is more important. We will however, build tooling and metrics for the users to be notified of these failures and let them handle this in the way they see fit.
 
-While Planned reparent is an operation where all the processes are running fine, Emergency reparent is called when something has gone wrong with the cluster. Because we call `DemotePrimary` in parallel with `StopReplicationAndBuildStatusMap`, we can run into a case wherein the primary tries to write something to the binlog after all the replicas have stopped replicating. If we were to run without semi-sync, then the primary could potentially commit a prepared transaction, and return a success to the vtgate trying to commit this transaction. The vtgate can then conclude that the transaction is safe to conclude and remove all the metadata information. However, on the new primary since the transaction commit didn't get replicated, it would re-prepare the transaction and would wait for a coordinator to either commit or rollback it, but that would never happen. Essentially we would have a transaction stuck in prepared state on a shard indefinitely. To avoid this situation, it is essential that we run with semi-sync, because this ensures that any write that is acknowledged as a success to the caller, would necessarily have to be replicated to at least one replica. This ensures that the transaction would also already be committed on the new primary.
+While Planned reparent is an operation where all the processes are running fine, Emergency reparent is called when something has gone wrong with the cluster. Because we call `DemotePrimary` in parallel with `StopReplicationAndBuildStatusMap`, we can run into a case wherein the primary tries to write something to the binlog after all the replicas have stopped replicating. If we were to run without semi-sync, then the primary could potentially commit a prepared transaction, and return a success to the VTGate trying to commit this transaction. The VTGate can then conclude that the transaction is safe to conclude and remove all the metadata information. However, on the new primary since the transaction commit didn't get replicated, it would re-prepare the transaction and would wait for a coordinator to either commit or rollback it, but that would never happen. Essentially we would have a transaction stuck in prepared state on a shard indefinitely. To avoid this situation, it is essential that we run with semi-sync, because this ensures that any write that is acknowledged as a success to the caller, would necessarily have to be replicated to at least one replica. This ensures that the transaction would also already be committed on the new primary.
 
 ### MySQL Restarts
 
 When MySQL restarts, it loses all the ongoing transactions which includes all the prepared transactions. This is because the transaction logs are not persistent across restarts. This is a MySQL limitation and there is no way to get around this. However, at the Vitess level we must ensure that we can commit the prepared transactions even in case of MySQL restarts without any failures.
 
-Vttablet has the code to detect MySQL failures and call `stateManager.checkMySQL()` which transitions the tablet to a NotConnected state. This prevents any writes from going through until the vttablet has transitioned back to a serving state.
+Vttablet has the code to detect MySQL failures and call `stateManager.checkMySQL()` which transitions the tablet to a NotConnected state. This prevents any writes from going through until the VTTablet has transitioned back to a serving state.
 
-However, we cannot rely on `checkMySQL` to ensure that no conflicting writes go through. This is because the time between MySQL restart and the vttablet transitioning to a NotConnected state can be large. During this time, the vttablet would still be accepting writes and some of them could potentially conflict with the prepared transactions.
+However, we cannot rely on `checkMySQL` to ensure that no conflicting writes go through. This is because the time between MySQL restart and the VTTablet transitioning to a NotConnected state can be large. During this time, the VTTablet would still be accepting writes and some of them could potentially conflict with the prepared transactions.
 
 To handle this, we rely on the fact that when MySQL restarts, it starts with super-read-only turned on. This means that no writes can go through. It is VTOrc that registers this as an issue and fixes it by calling `UndoDemotePrimary`. As part of that call, before we set MySQL to read-write, we ensure that all the prepared transactions are redone in the read_only state. We use the dba pool (that has admin permissions) to prepare the transactions. This is safe because we know that no conflicting writes can go through until we set MySQL to read-write. The code to set MySQL to read-write after redoing prepared transactions can be found in `TabletManager.redoPreparedTransactionsAndSetReadWrite()`.
 
@@ -599,7 +599,7 @@ There is no additional work needed for VTGate restarts. The atomic transaction w
 ### Online DDL
 
 During an Online DDL cutover, we need to ensure that all the prepared transactions on the online DDL table needs to be completed before we can proceed with the cutover.
-This is because the cutover involves a schema change and we cannot have any prepared transactions that are dependent on the old schema.
+This is because the cutover involves a schema change, and we cannot have any prepared transactions that are dependent on the old schema.
 
 As part of the cut-over process, Online DDL adds query rules to buffer new queries on the table.
 It then checks for any open prepared transaction on the table and waits for up to 100ms if found, then checks again.
@@ -612,9 +612,9 @@ The check on both sides prevents either the cutover from proceeding or the trans
 
 ### MoveTables
 
-The only step of a `MoveTables` workflow that needs to synchronize with atomic transactions is `SwitchTraffic` for writes. As part of this step, we want to disallow writes to only the tables involved. We use `DeniedTables` in `ShardInfo` to accomplish this. After we update the topo server with the new `DeniedTables`, we make all the vttablets refresh their topo to ensure that they've registered the change.
+The only step of a `MoveTables` workflow that needs to synchronize with atomic transactions is `SwitchTraffic` for writes. As part of this step, we want to disallow writes to only the tables involved. We use `DeniedTables` in `ShardInfo` to accomplish this. After we update the topo server with the new `DeniedTables`, we make all the VTTablets refresh their topo to ensure that they've registered the change.
 
-On vttablet, the `DeniedTables` are used to add query rules very similar to the ones in Online DDL. The only difference is that in Online DDL, we buffer the queries, but for `SwitchTraffic` we fail them altogether. Addition of these query rules, prevents any new atomic transactions from being prepared.
+On VTTablet, the `DeniedTables` are used to add query rules very similar to the ones in Online DDL. The only difference is that in Online DDL, we buffer the queries, but for `SwitchTraffic` we fail them altogether. Addition of these query rules, prevents any new atomic transactions from being prepared.
 
 Next, we try locking the tables to ensure no existing write is pending. This step blocks until all open prepared transactions have succeeded.
 
@@ -650,7 +650,7 @@ Currently, the user have to take the corrective actions for the transactions tha
 
 # Data Guarantees
 
-Although the above workflows are foolproof, they do rely on the data guarantees provided by the underlying systems and the fact that prepared transactions can get killed only together with vttablet.
+Although the above workflows are foolproof, they do rely on the data guarantees provided by the underlying systems and the fact that prepared transactions can get killed only together with VTTablet.
 In all the scenarios below, there is as possibility of irrecoverable data loss. But the system needs to alert correctly, and we must be able to make the best effort recovery and move on. 
 For now, these scenarios require operator intervention, but the system could be made to automatically perform these as we gain confidence.
 
@@ -683,7 +683,7 @@ This test should run over an extended period, potentially lasting a few days or
 * Online DDL operations
 
 ### Fuzzy tests
-A fuzzy test suite, running continous stream of multi-shard transactions and expecting events to be in specific sequence on terminating the long running test.
+A fuzzy test suite, running continuous stream of multi-shard transactions and expecting events to be in specific sequence on terminating the long-running test.
 
 ### Stress Tests
 A continuous stream of transactions (single and distributed) will be executed, with all successful commits recorded along with the expected rows. 
@@ -730,7 +730,7 @@ More details about the recent changes are present in the [RFC](https://github.co
 ## Exploratory Work
 MySQL XA was considered as an alternative to having RMs manage the transaction recovery logs and hold up the row locks until a commit or rollback occurs.
 
-There are currently 20+ open bugs on XA. On MySQL 8.0.33, reproduction steps were followed for all the bugs, and 8 still persist. Out of these 8 bugs, 4 have patches attached that resolve the issues when applied. 
+There are currently over 20 open bugs on XA. On MySQL 8.0.33, reproduction steps were followed for all these bugs, and 8 still persist. Out of these 8 bugs, 4 have patches attached that resolve the issues when applied. 
 For the remaining 4 issues, changes will need to be made either in the code or the workflow to ensure they are resolved.
 
 MySQL’s XA seems a probable candidate if we encounter issues with our implementation of handling distributed transactions that XA can resolve. XA's chatty API and no known big production deployment have kept us away from using it.