retry (#9480)

CHANGELOG

@@ -1,7 +1,9 @@
 v3.5.0-rc.5 (2019-XX-XX)
 ------------------------
+ 
+* Prevent rare cases of duplicate DDL actions being executed by Maintenance.
 
-* coordinator code was reporting rocksdb error codes, but not the associated detail message.
+* Coordinator code was reporting rocksdb error codes, but not the associated detail message.
   Corrected.
 
 * The keep alive timeout specified via --http.keep-alive-timeout is now being honored

arangod/Cluster/ActionBase.cpp

@@ -122,12 +122,12 @@ void ActionBase::createPreAction(std::shared_ptr<ActionDescription> const& descr
 
 /// @brief Retrieve pointer to action that should run before this one
 std::shared_ptr<Action> ActionBase::getPreAction() {
-  return (_preAction != nullptr) ? _feature.findAction(_preAction) : nullptr;
+  return (_preAction != nullptr) ? _feature.findFirstNotDoneAction(_preAction) : nullptr;
 }
 
 /// @brief Retrieve pointer to action that should run after this one
 std::shared_ptr<Action> ActionBase::getPostAction() {
-  return (_postAction != nullptr) ? _feature.findAction(_postAction) : nullptr;
+  return (_postAction != nullptr) ? _feature.findFirstNotDoneAction(_postAction) : nullptr;
 }
 
 // FIXMEMAINTENANCE: Code path could corrupt registry object because

arangod/Cluster/MaintenanceFeature.cpp

@@ -45,6 +45,15 @@ using namespace arangodb::maintenance;
 const uint32_t MaintenanceFeature::minThreadLimit = 2;
 const uint32_t MaintenanceFeature::maxThreadLimit = 64;
 
+
+namespace {
+
+bool findNotDoneActions(std::shared_ptr<maintenance::Action> const& action) {
+  return !action->done();
+}
+
+} // namespace
+
 MaintenanceFeature::MaintenanceFeature(application_features::ApplicationServer& server)
     : ApplicationFeature(server, "Maintenance"),
       _forceActivation(false),
@@ -198,10 +207,10 @@ Result MaintenanceFeature::addAction(std::shared_ptr<maintenance::Action> newAct
     size_t action_hash = newAction->hash();
     WRITE_LOCKER(wLock, _actionRegistryLock);
 
-    std::shared_ptr<Action> curAction = findActionHashNoLock(action_hash);
+    std::shared_ptr<Action> curAction = findFirstActionHashNoLock(action_hash, ::findNotDoneActions);
 
     // similar action not in the queue (or at least no longer viable)
-    if (curAction == nullptr || curAction->done()) {
+    if (!curAction) {
       if (newAction && newAction->ok()) {
         // Register action only if construction was ok
         registerAction(newAction, executeNow);
@@ -243,16 +252,13 @@ Result MaintenanceFeature::addAction(std::shared_ptr<maintenance::ActionDescript
   try {
     std::shared_ptr<Action> newAction;
 
-    // is there a known name field
-    auto find_it = description->get("name");
-
     size_t action_hash = description->hash();
     WRITE_LOCKER(wLock, _actionRegistryLock);
 
-    std::shared_ptr<Action> curAction = findActionHashNoLock(action_hash);
+    std::shared_ptr<Action> curAction = findFirstActionHashNoLock(action_hash, ::findNotDoneActions);
 
     // similar action not in the queue (or at least no longer viable)
-    if (!curAction || curAction->done()) {
+    if (!curAction) {
       newAction = createAndRegisterAction(description, executeNow);
 
       if (!newAction || !newAction->ok()) {
@@ -357,55 +363,45 @@ std::shared_ptr<Action> MaintenanceFeature::createAndRegisterAction(
   return newAction;
 }
 
-std::shared_ptr<Action> MaintenanceFeature::findAction(std::shared_ptr<ActionDescription> const description) {
+std::shared_ptr<Action> MaintenanceFeature::findFirstNotDoneAction(std::shared_ptr<ActionDescription> const& description) {
-  return findActionHash(description->hash());
+  return findFirstActionHash(description->hash(), ::findNotDoneActions);
 }
 
-std::shared_ptr<Action> MaintenanceFeature::findActionHash(size_t hash) {
+std::shared_ptr<Action> MaintenanceFeature::findFirstActionHash(size_t hash,
+    std::function<bool(std::shared_ptr<maintenance::Action> const&)> const& predicate) {
   READ_LOCKER(rLock, _actionRegistryLock);
 
-  return findActionHashNoLock(hash);
+  return findFirstActionHashNoLock(hash, predicate);
-}  // MaintenanceFeature::findActionHash
+}  
 
-std::shared_ptr<Action> MaintenanceFeature::findActionHashNoLock(size_t hash) {
+std::shared_ptr<Action> MaintenanceFeature::findFirstActionHashNoLock(size_t hash,
+    std::function<bool(std::shared_ptr<maintenance::Action> const&)> const& predicate) {
   // assert to test lock held?
 
-  std::shared_ptr<Action> ret_ptr;
+  for (auto const& action : _actionRegistry) {
-
+    if (action->hash() == hash && predicate(action)) {
-  for (auto action_it = _actionRegistry.begin();
+      return action;
-       _actionRegistry.end() != action_it && !ret_ptr; ++action_it) {
+    }
-    if ((*action_it)->hash() == hash) {
+  }
-      ret_ptr = *action_it;
+  return std::shared_ptr<Action>();
-    }  // if
+}
-  }    // for
-
-  return ret_ptr;
-
-}  // MaintenanceFeature::findActionHashNoLock
 
 std::shared_ptr<Action> MaintenanceFeature::findActionId(uint64_t id) {
   READ_LOCKER(rLock, _actionRegistryLock);
 
   return findActionIdNoLock(id);
-}  // MaintenanceFeature::findActionId
+} 
 
 std::shared_ptr<Action> MaintenanceFeature::findActionIdNoLock(uint64_t id) {
   // assert to test lock held?
 
-  std::shared_ptr<Action> ret_ptr;
+  for (auto const& action : _actionRegistry) {
-
+    if (action->id() == id) {
-  for (auto action_it = _actionRegistry.begin();
+      return action;
-       _actionRegistry.end() != action_it && !ret_ptr; ++action_it) {
+    }
-    if ((*action_it)->id() == id) {
+  }
-      // should we return the first match here or the last match?
+  return std::shared_ptr<Action>();
-      // if first, we could simply add a break
+}
-      ret_ptr = *action_it;
-    }  // if
-  }    // for
-
-  return ret_ptr;
-
-}  // MaintenanceFeature::findActionIdNoLock
 
 std::shared_ptr<Action> MaintenanceFeature::findReadyAction(std::unordered_set<std::string> const& labels) {
   std::shared_ptr<Action> ret_ptr;
@@ -458,7 +454,7 @@ std::shared_ptr<Action> MaintenanceFeature::findReadyAction(std::unordered_set<s
 
   return ret_ptr;
 
-}  // MaintenanceFeature::findReadyAction
+} 
 
 VPackBuilder MaintenanceFeature::toVelocyPack() const {
   VPackBuilder vb;
@@ -648,20 +644,6 @@ arangodb::Result MaintenanceFeature::storeIndexError(
   return Result();
 }
 
-arangodb::Result MaintenanceFeature::indexErrors(
-    std::string const& database, std::string const& collection, std::string const& shard,
-    std::map<std::string, std::shared_ptr<VPackBuffer<uint8_t>>>& error) const {
-  std::string key = database + SLASH + collection + SLASH + shard;
-
-  MUTEX_LOCKER(guard, _ieLock);
-  auto const& it = _indexErrors.find(key);
-  if (it != _indexErrors.end()) {
-    error = it->second;
-  }
-
-  return Result();
-}
-
 template <typename T>
 std::ostream& operator<<(std::ostream& os, std::set<T> const& st) {
   size_t j = 0;

arangod/Cluster/MaintenanceFeature.h

@@ -153,10 +153,10 @@ class MaintenanceFeature : public application_features::ApplicationFeature {
   uint32_t getSecondsActionsBlock() const { return _secondsActionsBlock; };
 
   /**
-   * @brief Find and return found action or nullptr
+   * @brief Find and return first found not-done action or nullptr
    * @param desc Description of sought action
    */
-  std::shared_ptr<maintenance::Action> findAction(std::shared_ptr<maintenance::ActionDescription> const desc);
+  std::shared_ptr<maintenance::Action> findFirstNotDoneAction(std::shared_ptr<maintenance::ActionDescription> const& desc);
 
   /**
    * @brief add index error to bucket
@@ -173,20 +173,6 @@ class MaintenanceFeature : public application_features::ApplicationFeature {
                                    std::string const& shard, std::string const& indexId,
                                    std::shared_ptr<VPackBuffer<uint8_t>> error);
 
-  /**
-   * @brief get all pending index errors for a specific shard
-   *
-   * @param  database     database
-   * @param  collection   collection
-   * @param  shard        shard
-   * @param  errors       errrors map returned to caller
-   *
-   * @return success
-   */
-  arangodb::Result indexErrors(
-      std::string const& database, std::string const& collection, std::string const& shard,
-      std::map<std::string, std::shared_ptr<VPackBuffer<uint8_t>>>& errors) const;
-
   /**
    * @brief remove 1+ errors from index error bucket
    *        Errors are removed by phaseOne, as soon as indexes no longer in plan
@@ -312,17 +298,19 @@ class MaintenanceFeature : public application_features::ApplicationFeature {
    */
   void delShardVersion(std::string const& shardId);
 
- protected:
+ private:
   /// @brief common code used by multiple constructors
   void init();
 
-  /// @brief Search for action by hash
+  /// @brief Search for first action matching hash and predicate
-  /// @return shared pointer to action object if exists, _actionRegistry.end() if not
+  /// @return shared pointer to action object if exists, empty shared_ptr if not
-  std::shared_ptr<maintenance::Action> findActionHash(size_t hash);
+  std::shared_ptr<maintenance::Action> findFirstActionHash(size_t hash,
+                                                           std::function<bool(std::shared_ptr<maintenance::Action> const&)> const& predicate);
 
-  /// @brief Search for action by hash (but lock already held by caller)
+  /// @brief Search for first action matching hash and predicate (with lock already held by caller)
-  /// @return shared pointer to action object if exists, nullptr if not
+  /// @return shared pointer to action object if exists, empty shared_ptr if not
-  std::shared_ptr<maintenance::Action> findActionHashNoLock(size_t hash);
+  std::shared_ptr<maintenance::Action> findFirstActionHashNoLock(size_t hash,
+                                                                 std::function<bool(std::shared_ptr<maintenance::Action> const&)> const& predicate);
 
   /// @brief Search for action by Id
   /// @return shared pointer to action object if exists, nullptr if not
@@ -332,6 +320,8 @@ class MaintenanceFeature : public application_features::ApplicationFeature {
   /// @return shared pointer to action object if exists, nullptr if not
   std::shared_ptr<maintenance::Action> findActionIdNoLock(uint64_t hash);
 
+ protected:
+
   /// @brief option for forcing this feature to always be enable - used by the catch tests
   bool _forceActivation;