Added special lock for local data. Use read and write locking. Do not hold read lock during agency transactions. (#9277)

arangod/Cluster/FollowerInfo.cpp

@@ -30,14 +30,6 @@
 
 using namespace arangodb;
 
-////////////////////////////////////////////////////////////////////////////////
-/// @brief get information about current followers of a shard.
-////////////////////////////////////////////////////////////////////////////////
-
-std::shared_ptr<std::vector<ServerID> const> FollowerInfo::get() const {
-  MUTEX_LOCKER(locker, _mutex);
-  return _followers;
-}
 
 ////////////////////////////////////////////////////////////////////////////////
 /// @brief change JSON under
@@ -98,23 +90,29 @@ static VPackBuilder newShardEntry(VPackSlice oldValue, ServerID const& sid, bool
 ////////////////////////////////////////////////////////////////////////////////
 
 void FollowerInfo::add(ServerID const& sid) {
-  MUTEX_LOCKER(locker, _mutex);
 
-  // First check if there is anything to do:
+  MUTEX_LOCKER(locker, _agencyMutex);
-  for (auto const& s : *_followers) {
+
-    if (s == sid) {
+  std::shared_ptr<std::vector<ServerID>> v;
-      return;  // Do nothing, if follower already there
+
+  {
+    WRITE_LOCKER(writeLocker, _dataLock);
+    // First check if there is anything to do:
+    for (auto const& s : *_followers) {
+      if (s == sid) {
+        return;  // Do nothing, if follower already there
+      }
     }
+    // Fully copy the vector:
+    v = std::make_shared<std::vector<ServerID>>(*_followers);
+    v->push_back(sid);  // add a single entry
+    _followers = v;     // will cast to std::vector<ServerID> const
+  #ifdef DEBUG_SYNC_REPLICATION
+    if (!AgencyCommManager::MANAGER) {
+      return;
+    }
+  #endif
   }
-  // Fully copy the vector:
-  auto v = std::make_shared<std::vector<ServerID>>(*_followers);
-  v->push_back(sid);  // add a single entry
-  _followers = v;     // will cast to std::vector<ServerID> const
-#ifdef DEBUG_SYNC_REPLICATION
-  if (!AgencyCommManager::MANAGER) {
-    return;
-  }
-#endif
   // Now tell the agency, path is
   //   Current/Collections/<dbName>/<collectionID>/<shardID>
   std::string path = "Current/Collections/";
@@ -192,7 +190,10 @@ bool FollowerInfo::remove(ServerID const& sid) {
   LOG_TOPIC("ce460", DEBUG, Logger::CLUSTER)
       << "Removing follower " << sid << " from " << _docColl->name();
 
-  MUTEX_LOCKER(locker, _mutex);
+  MUTEX_LOCKER(locker, _agencyMutex);
+  WRITE_LOCKER(writeLocker, _dataLock); // the data lock has to be locked until this function completes
+                                        // because if the agency communication does not work
+                                        // local data is modified again.
 
   // First check if there is anything to do:
   bool found = false;
@@ -294,7 +295,7 @@ bool FollowerInfo::remove(ServerID const& sid) {
 //////////////////////////////////////////////////////////////////////////////
 
 void FollowerInfo::clear() {
-  MUTEX_LOCKER(locker, _mutex);
+  WRITE_LOCKER(writeLocker, _dataLock);
   auto v = std::make_shared<std::vector<ServerID>>();
   _followers = v;  // will cast to std::vector<ServerID> const
 }
@@ -304,7 +305,7 @@ void FollowerInfo::clear() {
 //////////////////////////////////////////////////////////////////////////////
 
 bool FollowerInfo::contains(ServerID const& sid) const {
-  MUTEX_LOCKER(locker, _mutex);
+  READ_LOCKER(readLocker, _dataLock);
   for (auto const& s : *_followers) {
     if (s == sid) {
       return true;

arangod/Cluster/FollowerInfo.h

@@ -26,6 +26,9 @@
 #define ARANGOD_CLUSTER_FOLLOWER_INFO_H 1
 
 #include "ClusterInfo.h"
+#include "Basics/Mutex.h"
+#include "Basics/ReadWriteLock.h"
+#include "Basics/WriteLocker.h"
 
 namespace arangodb {
 
@@ -35,7 +38,12 @@ namespace arangodb {
 
 class FollowerInfo {
   std::shared_ptr<std::vector<ServerID> const> _followers;
-  mutable Mutex _mutex;
+
+  // The agencyMutex is used to synchronise access to the agency.
+  // the _dataLock is used to sync the access to local data.
+  // The agencyMutex is always locked before the _dataLock is locked.
+  mutable Mutex _agencyMutex;
+  mutable arangodb::basics::ReadWriteLock _dataLock;
   arangodb::LogicalCollection* _docColl;
   std::string _theLeader;
   // if the latter is empty, then we are leading
@@ -45,11 +53,14 @@ class FollowerInfo {
   explicit FollowerInfo(arangodb::LogicalCollection* d)
       : _followers(new std::vector<ServerID>()), _docColl(d), _theLeaderTouched(false) {}
 
-  //////////////////////////////////////////////////////////////////////////////
+  ////////////////////////////////////////////////////////////////////////////////
   /// @brief get information about current followers of a shard.
-  //////////////////////////////////////////////////////////////////////////////
+  ////////////////////////////////////////////////////////////////////////////////
 
-  std::shared_ptr<std::vector<ServerID> const> get() const;
+  std::shared_ptr<std::vector<ServerID> const> get() const {
+    READ_LOCKER(readLocker, _dataLock);
+    return _followers;
+  }
 
   //////////////////////////////////////////////////////////////////////////////
   /// @brief add a follower to a shard, this is only done by the server side
@@ -87,7 +98,7 @@ class FollowerInfo {
   //////////////////////////////////////////////////////////////////////////////
 
   void setTheLeader(std::string const& who) {
-    MUTEX_LOCKER(locker, _mutex);
+    WRITE_LOCKER(writeLocker, _dataLock);
     _theLeader = who;
     _theLeaderTouched = true;
   }
@@ -97,7 +108,7 @@ class FollowerInfo {
   //////////////////////////////////////////////////////////////////////////////
 
   std::string getLeader() const {
-    MUTEX_LOCKER(locker, _mutex);
+    READ_LOCKER(readLocker, _dataLock);
     return _theLeader;
   }
 
@@ -106,7 +117,7 @@ class FollowerInfo {
   //////////////////////////////////////////////////////////////////////////////
 
   bool getLeaderTouched() const {
-    MUTEX_LOCKER(locker, _mutex);
+    READ_LOCKER(readLocker, _dataLock);
     return _theLeaderTouched;
   }
 };