Merge pull request #2722 from Expensify/main

Update expensify_prod branch

Author: tylerkaraszewski

BedrockCommand.cpp

@@ -245,9 +245,7 @@ void BedrockCommand::finalizeTimingInfo()
     uint64_t blockingPostProcessTotal = 0;
     uint64_t commitWorkerTotal = 0;
     uint64_t blockingCommitWorkerTotal = 0;
-    uint64_t commitSyncTotal = 0;
     uint64_t queueWorkerTotal = 0;
-    uint64_t queueSyncTotal = 0;
     uint64_t queueBlockingTotal = 0;
     uint64_t queuePageLockTotal = 0;
     for (const auto& entry: timingInfo) {
@@ -276,14 +274,10 @@ void BedrockCommand::finalizeTimingInfo()
         } else if (get<0>(entry) == BLOCKING_COMMIT_WORKER) {
             commitWorkerTotal += get<2>(entry) - get<1>(entry);
             blockingCommitWorkerTotal += get<2>(entry) - get<1>(entry);
-        } else if (get<0>(entry) == COMMIT_SYNC) {
-            commitSyncTotal += get<2>(entry) - get<1>(entry);
         } else if (get<0>(entry) == QUEUE_WORKER) {
             queueWorkerTotal += get<2>(entry) - get<1>(entry);
         } else if (get<0>(entry) == QUEUE_BLOCKING) {
             queueBlockingTotal += get<2>(entry) - get<1>(entry);
-        } else if (get<0>(entry) == QUEUE_SYNC) {
-            queueSyncTotal += get<2>(entry) - get<1>(entry);
         } else if (get<0>(entry) == QUEUE_PAGE_LOCK) {
             queuePageLockTotal += get<2>(entry) - get<1>(entry);
         }
@@ -293,8 +287,8 @@ void BedrockCommand::finalizeTimingInfo()
     uint64_t totalTime = STimeNow() - creationTime;
 
     // Time that wasn't accounted for in all the other metrics.
-    uint64_t unaccountedTime = totalTime - (prePeekTotal + peekTotal + processTotal + postProcessTotal + commitWorkerTotal + commitSyncTotal +
-        escalationTimeUS + queueWorkerTotal + queueBlockingTotal + queueSyncTotal + queuePageLockTotal);
+    uint64_t unaccountedTime = totalTime - (prePeekTotal + peekTotal + processTotal + postProcessTotal + commitWorkerTotal +
+        escalationTimeUS + queueWorkerTotal + queueBlockingTotal + queuePageLockTotal);
 
     uint64_t exclusiveTransactionLockTime = blockingPeekTotal + blockingProcessTotal + blockingCommitWorkerTotal;
     uint64_t blockingCommitThreadTime = exclusiveTransactionLockTime + blockingPrePeekTotal + blockingPostProcessTotal;
@@ -357,11 +351,9 @@ void BedrockCommand::finalizeTimingInfo()
           "blockingCommitThreadTime:" << blockingCommitThreadTime / 1000 << ", "
           "exclusiveTransactionLockTime:" << exclusiveTransactionLockTime / 1000 <<
           ". Commit: "
-          "worker:" << commitWorkerTotal / 1000 << ", "
-          "sync:" << commitSyncTotal / 1000 <<
+          "worker:" << commitWorkerTotal / 1000 <<
           ". Queue: "
           "worker:" << queueWorkerTotal / 1000 << ", "
-          "sync:" << queueSyncTotal / 1000 << ", "
           "blocking:" << queueBlockingTotal / 1000 << ", "
           "pageLock:" << queuePageLockTotal / 1000 << ", "
           "escalation:" << escalationTimeUS / 1000 <<

BedrockCommand.h

@@ -15,9 +15,7 @@ class BedrockCommand : public SQLiteCommand {
         PROCESS,
         POSTPROCESS,
         COMMIT_WORKER,
-        COMMIT_SYNC,
         QUEUE_WORKER,
-        QUEUE_SYNC,
         QUEUE_BLOCKING,
         QUEUE_PAGE_LOCK,
 
@@ -42,6 +40,12 @@ class BedrockCommand : public SQLiteCommand {
     static const uint64_t DEFAULT_TIMEOUT_FORGET = 60'000 * 60; // 1 hour for `connection: forget` commands.
     static const uint64_t DEFAULT_PROCESS_TIMEOUT = 5'000; // 5 seconds.
 
+    // How long a command named in `-synchronousCommands` may hold the blocking commit thread. That thread owns the
+    // commit lock for its whole transaction, and SQLiteNode::_changeState waits on the same lock with no timeout, so
+    // this also bounds how long a node state change can be stalled. It has to stay well under the 30 second peer
+    // receive timeout that would otherwise cost us the leader.
+    static const uint64_t SYNCHRONOUS_COMMAND_TIMEOUT = 20'000; // 20 seconds.
+
     // Constructor to initialize via a request object (by move).
     BedrockCommand(SQLiteCommand&& baseCommand, BedrockPlugin* plugin, bool escalateImmediately_ = false);
 
@@ -306,6 +310,11 @@ class BedrockCommand : public SQLiteCommand {
     // in `process` instead of peek, as it will always be escalated to leader
     const bool escalateImmediately;
 
+    // True if this command was named in `-synchronousCommands`. Such commands run on the blocking commit thread, so
+    // they're serialized against each other and against every other write, and they get SYNCHRONOUS_COMMAND_TIMEOUT
+    // rather than the shorter DEFAULT_PROCESS_TIMEOUT that thread otherwise imposes.
+    bool isSynchronous = false;
+
     // Setting this to `true` will cause this command to abort immediately, even in the middle of a slow database query.
     // NOTE: currently, this will not abort from every case where a command could be stuck, but only in DB queries.
     atomic<bool> shouldAbort = false;

BedrockCore.cpp

@@ -167,8 +167,9 @@ BedrockCore::RESULT BedrockCore::peekCommand(unique_ptr<BedrockCommand>& command
                 STHROW("501 Failed to begin " + (exclusive ? "exclusive"s : "shared"s) + " transaction");
             }
 
-            if (exclusive && command->writeConsistency != SQLiteNode::QUORUM) {
-                decreaseCommandTimeout(command, BedrockCommand::DEFAULT_PROCESS_TIMEOUT);
+            if (exclusive) {
+                decreaseCommandTimeout(command, command->isSynchronous ? BedrockCommand::SYNCHRONOUS_COMMAND_TIMEOUT
+                                                                       : BedrockCommand::DEFAULT_PROCESS_TIMEOUT);
             }
 
             // We start the timer here to avoid including the time spent acquiring the lock _sharedData.commitLock
@@ -269,8 +270,9 @@ BedrockCore::RESULT BedrockCore::processCommand(unique_ptr<BedrockCommand>& comm
                 STHROW("501 Failed to begin " + (exclusive ? "exclusive"s : "shared"s) + " transaction");
             }
 
-            if (exclusive && command->writeConsistency != SQLiteNode::QUORUM) {
-                decreaseCommandTimeout(command, BedrockCommand::DEFAULT_PROCESS_TIMEOUT);
+            if (exclusive) {
+                decreaseCommandTimeout(command, command->isSynchronous ? BedrockCommand::SYNCHRONOUS_COMMAND_TIMEOUT
+                                                                       : BedrockCommand::DEFAULT_PROCESS_TIMEOUT);
             }
         }