patch collection counts on the slave in case it is off (#7147)

arangod/RestHandler/RestDebugHandler.cpp

@@ -46,6 +46,12 @@ RestStatus RestDebugHandler::execute() {
 
   // execute one of the CRUD methods
   switch (type) {
+    case rest::RequestType::GET: {
+      VPackBuilder result;
+      result.add(VPackValue(TRI_CanUseFailurePointsDebugging()));
+      generateResult(rest::ResponseCode::OK, result.slice());
+      return RestStatus::DONE;
+    }
     case rest::RequestType::DELETE_REQ:
       if (len == 1) {
         TRI_ClearFailurePointsDebugging();

arangod/RocksDBEngine/RocksDBIncrementalSync.cpp

@@ -30,6 +30,7 @@
 #include "RocksDBEngine/RocksDBIterators.h"
 #include "RocksDBEngine/RocksDBKey.h"
 #include "RocksDBEngine/RocksDBPrimaryIndex.h"
+#include "RocksDBEngine/RocksDBTransactionCollection.h"
 #include "SimpleHttpClient/SimpleHttpClient.h"
 #include "SimpleHttpClient/SimpleHttpResult.h"
 #include "StorageEngine/PhysicalCollection.h"
@@ -848,13 +849,19 @@ Result handleSyncKeysRocksDB(DatabaseInitialSyncer& syncer,
       uint64_t numberDocumentsAfterSync = documentsFound + stats.numDocsInserted - (stats.numDocsRemoved - numberDocumentsRemovedBeforeStart);
       uint64_t numberDocumentsDueToCounter = col->numberDocuments(&trx, transaction::CountType::Normal);
       syncer.setProgress(std::string("number of remaining documents in collection '") + col->name() + "' " + std::to_string(numberDocumentsAfterSync) + ", number of documents due to collection count: " + std::to_string(numberDocumentsDueToCounter));
+
       if (numberDocumentsAfterSync != numberDocumentsDueToCounter) {
-        LOG_TOPIC(DEBUG, Logger::REPLICATION) << "number of remaining documents in collection '" + col->name() + "' is " + std::to_string(numberDocumentsAfterSync) + " and differs from number of documents returned by collection count " + std::to_string(numberDocumentsDueToCounter);
+        LOG_TOPIC(WARN, Logger::REPLICATION) << "number of remaining documents in collection '" + col->name() + "' is " + std::to_string(numberDocumentsAfterSync) + " and differs from number of documents returned by collection count " + std::to_string(numberDocumentsDueToCounter);
+
+        // patch the document counter of the collection and the transaction
+        int64_t diff = static_cast<int64_t>(numberDocumentsAfterSync) - static_cast<int64_t>(numberDocumentsDueToCounter);
+        static_cast<RocksDBCollection*>(trx.documentCollection()->getPhysical())->adjustNumberDocuments(0, diff);
       }
     }
 
     res = trx.commit();
-    if (!res.ok()) {
+
+    if (res.fail()) {
       return res;
     }
   }

arangod/RocksDBEngine/RocksDBTransactionCollection.cpp

@@ -312,7 +312,7 @@ void RocksDBTransactionCollection::abortCommit(uint64_t trxId) {
 
 void RocksDBTransactionCollection::commitCounts(uint64_t trxId,
                                                 uint64_t commitSeq) {
-    TRI_ASSERT(_collection != nullptr);
+  TRI_ASSERT(_collection != nullptr);
 
   // Update the collection count
   int64_t const adjustment = _numInserts - _numRemoves;
@@ -331,6 +331,7 @@ void RocksDBTransactionCollection::commitCounts(uint64_t trxId,
   // Update the index estimates.
   for (auto& pair : _trackedIndexOperations) {
     auto idx = _collection->lookupIndex(pair.first);
+
     if (idx == nullptr) {
       TRI_ASSERT(false); // Index reported estimates, but does not exist
       continue;

arangod/RocksDBEngine/RocksDBTransactionState.cpp

@@ -340,6 +340,10 @@ arangodb::Result RocksDBTransactionState::internalCommit() {
         // we need this in case of an intermediate commit. The number of
         // initial documents is adjusted and numInserts / removes is set to 0
         // index estimator updates are buffered
+        TRI_IF_FAILURE("RocksDBCommitCounts") {
+          committed = true;
+          continue;
+        }
         collection->commitCounts(id(), postCommitSeq);
         committed = true;
       }
@@ -367,6 +371,9 @@ arangodb::Result RocksDBTransactionState::internalCommit() {
                _rocksTransaction->GetNumDeletes() == 0);
 
     for (auto& trxCollection : _collections) {
+      TRI_IF_FAILURE("RocksDBCommitCounts") {
+        continue;
+      }
       RocksDBTransactionCollection* collection =
           static_cast<RocksDBTransactionCollection*>(trxCollection);
       // We get here if we have filled indexes. So let us commit counts and

arangod/RocksDBEngine/RocksDBTransactionState.h

@@ -64,7 +64,6 @@ struct Options;
 
 }
 
-class TransactionCollection;
 class RocksDBMethods;
 
 /// @brief transaction type

arangod/Transaction/Methods.h

@@ -436,9 +436,8 @@ class Methods {
   virtual bool isInaccessibleCollectionId(TRI_voc_cid_t /*cid*/) { return false; }
   virtual bool isInaccessibleCollection(std::string const& /*cid*/) { return false; }
 #endif
-
+  
  private:
-
   /// @brief build a VPack object with _id, _key and _rev and possibly
   /// oldRef (if given), the result is added to the builder in the
   /// argument as a single object.
@@ -510,6 +509,9 @@ class Methods {
                                                  OperationOptions const& options);
 
  protected:
+  /// @brief return the transaction collection for a document collection
+  ENTERPRISE_VIRT TransactionCollection* trxCollection(TRI_voc_cid_t cid,
+                               AccessMode::Type type = AccessMode::Type::READ) const;
 
   OperationResult countCoordinator(std::string const& collectionName,
                                    CountType type);
@@ -519,10 +521,6 @@ class Methods {
 
   OperationResult countLocal(std::string const& collectionName, CountType type);
 
-  /// @brief return the transaction collection for a document collection
-  ENTERPRISE_VIRT TransactionCollection* trxCollection(TRI_voc_cid_t cid,
-                               AccessMode::Type type = AccessMode::Type::READ) const;
-
   /// @brief return the collection
   arangodb::LogicalCollection* documentCollection(
       TransactionCollection const*) const;

arangod/Utils/SingleCollectionTransaction.h

@@ -40,44 +40,26 @@ class Context;
 class SingleCollectionTransaction final : public transaction::Methods {
  public:
 
-  //////////////////////////////////////////////////////////////////////////////
   /// @brief create the transaction, using a data-source
-  //////////////////////////////////////////////////////////////////////////////
   SingleCollectionTransaction(
     std::shared_ptr<transaction::Context> const& transactionContext,
     LogicalDataSource const& collection,
     AccessMode::Type accessType
   );
 
-  //////////////////////////////////////////////////////////////////////////////
   /// @brief create the transaction, using a collection name
-  //////////////////////////////////////////////////////////////////////////////
   SingleCollectionTransaction(std::shared_ptr<transaction::Context> const&,
                               std::string const&, AccessMode::Type);
 
-  //////////////////////////////////////////////////////////////////////////////
   /// @brief end the transaction
-  //////////////////////////////////////////////////////////////////////////////
   ~SingleCollectionTransaction() = default;
 
- private:
-  //////////////////////////////////////////////////////////////////////////////
-  /// @brief get the underlying transaction collection
-  //////////////////////////////////////////////////////////////////////////////
-  TransactionCollection* resolveTrxCollection();
-
- public:
-
-  //////////////////////////////////////////////////////////////////////////////
   /// @brief get the underlying document collection
   /// note that we have two identical versions because this is called
   /// in two different situations
-  //////////////////////////////////////////////////////////////////////////////
   LogicalCollection* documentCollection();
 
-  //////////////////////////////////////////////////////////////////////////////
   /// @brief get the underlying collection's id
-  //////////////////////////////////////////////////////////////////////////////
   inline TRI_voc_cid_t cid() const { return _cid; }
 
 #ifdef USE_ENTERPRISE
@@ -89,35 +71,27 @@ class SingleCollectionTransaction final : public transaction::Methods {
     return _cid;
   }
 
-  //////////////////////////////////////////////////////////////////////////////
   /// @brief get the underlying collection's name
-  //////////////////////////////////////////////////////////////////////////////
-
   std::string name();
 
  private:
+  
+  /// @brief get the underlying transaction collection
+  TransactionCollection* resolveTrxCollection();
 
-  //////////////////////////////////////////////////////////////////////////////
   /// @brief collection id
-  //////////////////////////////////////////////////////////////////////////////
   TRI_voc_cid_t _cid;
 
-  //////////////////////////////////////////////////////////////////////////////
   /// @brief trxCollection cache
-  //////////////////////////////////////////////////////////////////////////////
   TransactionCollection* _trxCollection;
 
-  //////////////////////////////////////////////////////////////////////////////
   /// @brief LogicalCollection* cache
-  //////////////////////////////////////////////////////////////////////////////
   LogicalCollection* _documentCollection;
 
-  //////////////////////////////////////////////////////////////////////////////
   /// @brief collection access type
-  //////////////////////////////////////////////////////////////////////////////
   AccessMode::Type _accessType;
 };
 
 }
 
-#endif
+#endif

etc/testing/arangod-common.conf

@@ -1,5 +1,5 @@
 [log]
-line-number = true
+line-number = false
 force-direct = false
 level = info
 level = replication=warn

tests/js/server/replication/replication-sync.js

@@ -1,5 +1,5 @@
 /* jshint globalstrict:false, strict:false, unused: false */
-/* global assertEqual, assertTrue, assertFalse, arango, ARGUMENTS */
+/* global arango, assertEqual, assertTrue, assertFalse, arango, ARGUMENTS */
 
 // //////////////////////////////////////////////////////////////////////////////
 // / @brief test the sync method of the replication
@@ -57,13 +57,11 @@ const connectToSlave = function () {
 };
 
 const collectionChecksum = function (name) {
-  var c = db._collection(name).checksum(true, true);
-  return c.checksum;
+  return db._collection(name).checksum(true, true).checksum;
 };
 
 const collectionCount = function (name) {
-  var res = db._collection(name).count();
-  return res;
+  return db._collection(name).count();
 };
 
 const compare = function (masterFunc, slaveInitFunc, slaveCompareFunc, incremental) {
@@ -96,6 +94,126 @@ function BaseTestConfig () {
   'use strict';
 
   return {
+    
+    // //////////////////////////////////////////////////////////////////////////////
+    // / @brief test existing collection
+    // //////////////////////////////////////////////////////////////////////////////
+
+    testExistingPatchBrokenSlaveCounters1: function () {
+      // can only use this with failure tests enabled and RocksDB engine
+      if (db._engine().name !== "rocksdb") {
+        return;
+      }
+          
+      let r = arango.GET("/_db/" + db._name() + "/_admin/debug/failat");
+      if (String(r) === "false") {
+        return;
+      }
+
+      connectToMaster();
+
+      compare(
+        function (state) {
+          let c = db._create(cn);
+          let docs = [];
+
+          for (let i = 0; i < 5000; ++i) {
+            docs.push({ value: i });
+          }
+          c.insert(docs);
+
+          state.checksum = collectionChecksum(cn);
+          state.count = collectionCount(cn);
+          assertEqual(5000, state.count);
+        },
+        function (state) {
+          //  already create the collection on the slave
+          replication.syncCollection(cn, {
+            endpoint: masterEndpoint,
+            incremental: false
+          });
+          // collection present on slave now
+          var c = db._collection(cn);
+          assertEqual(5000, c.count());
+          assertEqual(5000, c.toArray().length);
+          arango.PUT_RAW("/_admin/debug/failat/RocksDBCommitCounts", "");
+          c.insert({});
+          arango.DELETE_RAW("/_admin/debug/failat", "");
+          assertEqual(5000, c.count());
+          assertEqual(5001, c.toArray().length);
+        },
+        function (state) {
+          assertEqual(state.count, collectionCount(cn));
+          assertEqual(state.checksum, collectionChecksum(cn));
+          // now validate counters
+          let c = db._collection(cn);
+          assertEqual(5000, c.toArray().length);
+          assertEqual(5000, c.count());
+        },
+        true
+      );
+    },
+    
+    testExistingPatchBrokenSlaveCounters2: function () {
+      // can only use this with failure tests enabled and RocksDB engine
+      if (db._engine().name !== "rocksdb") {
+        return;
+      }
+          
+      let r = arango.GET("/_db/" + db._name() + "/_admin/debug/failat");
+      if (String(r) === "false") {
+        return;
+      }
+
+      connectToMaster();
+
+      compare(
+        function (state) {
+          let c = db._create(cn);
+          let docs = [];
+
+          for (let i = 0; i < 10000; ++i) {
+            docs.push({ value: i, _key: "test" + i });
+          }
+          c.insert(docs);
+
+          state.checksum = collectionChecksum(cn);
+          state.count = collectionCount(cn);
+          assertEqual(10000, state.count);
+        },
+        function (state) {
+          //  already create the collection on the slave
+          replication.syncCollection(cn, {
+            endpoint: masterEndpoint,
+            incremental: false
+          });
+          
+          // collection present on slave now
+          var c = db._collection(cn);
+          for (let i = 0; i < 10000; i += 10) {
+            c.remove("test" + i);
+          }
+          assertEqual(9000, c.count());
+          assertEqual(9000, c.toArray().length);
+          arango.PUT_RAW("/_admin/debug/failat/RocksDBCommitCounts", "");
+          for (let i = 0; i < 100; ++i) {
+            c.insert({ _key: "testmann" + i });
+          }
+          arango.DELETE_RAW("/_admin/debug/failat", "");
+          assertEqual(9000, c.count());
+          assertEqual(9100, c.toArray().length);
+        },
+        function (state) {
+          assertEqual(state.count, collectionCount(cn));
+          assertEqual(state.checksum, collectionChecksum(cn));
+          // now validate counters
+          let c = db._collection(cn);
+          assertEqual(10000, c.count());
+          assertEqual(10000, c.toArray().length);
+        },
+        true
+      );
+    },
 
     // //////////////////////////////////////////////////////////////////////////////
     // / @brief test existing collection
@@ -152,7 +270,7 @@ function BaseTestConfig () {
       assertEqual(st.count, collectionCount(cn));
       assertEqual(st.checksum, collectionChecksum(cn));
     },
-
+    
     // //////////////////////////////////////////////////////////////////////////////
     // / @brief test existing collection
     // //////////////////////////////////////////////////////////////////////////////
@@ -1250,7 +1368,7 @@ function BaseTestConfig () {
         true
       );
     },
-
+    
     // //////////////////////////////////////////////////////////////////////////////
     // / @brief test with existing documents - same on the slave but different keys
     // //////////////////////////////////////////////////////////////////////////////
@@ -1452,7 +1570,7 @@ function BaseTestConfig () {
         true
       );
     }
-
+  
   };
 }