arangodb · jsteemann · Dec 12, 2018 · Dec 11, 2018 · Dec 11, 2018 · Dec 11, 2018
diff --git a/arangod/VocBase/KeyGenerator.cpp b/arangod/VocBase/KeyGenerator.cpp
@@ -132,33 +132,23 @@ class TraditionalKeyGenerator : public KeyGenerator {
  public:
   /// @brief create the generator
   explicit TraditionalKeyGenerator(bool allowUserKeys)
-    : KeyGenerator(allowUserKeys),
-      _lastValue(0) {}
+    : KeyGenerator(allowUserKeys) {}
 
   bool hasDynamicState() const override { return true; }
-
+  
   /// @brief generate a key
   std::string generate() override final {
     uint64_t tick = generateValue();
 
-    {
-      MUTEX_LOCKER(mutexLocker, _lock);
-
-      if (tick == UINT64_MAX || _lastValue >= UINT64_MAX - 1ULL) {
-        // oops, out of keys!
-        return std::string();
-      }
-
-      if (tick <= _lastValue) {
-        tick = ++_lastValue;
-      } else {
-        _lastValue = tick;
-      }
+    if (ADB_UNLIKELY(tick == 0)) {
+      // unlikely case we have run out of keys
+      // returning an empty string will trigger an error on the call site
+      return std::string();
     }
 
     return arangodb::basics::StringUtils::itoa(tick);
   }
-  
+
   /// @brief validate a key
   int validate(char const* p, size_t length, bool isRestore) override {
     int res = KeyGenerator::validate(p, length, isRestore);
@@ -180,12 +170,7 @@ class TraditionalKeyGenerator : public KeyGenerator {
       uint64_t value = NumberUtils::atoi_zero<uint64_t>(p, p + length);
 
       if (value > 0) {
-        MUTEX_LOCKER(mutexLocker, _lock);
-
-        if (value > _lastValue) {
-          // and update our last value
-          _lastValue = value;
-        }
+        track(value);
       }
     }
   }
@@ -194,76 +179,123 @@ class TraditionalKeyGenerator : public KeyGenerator {
   void toVelocyPack(arangodb::velocypack::Builder& builder) const override {
     KeyGenerator::toVelocyPack(builder);
     builder.add("type", VPackValue("traditional"));
-
-    MUTEX_LOCKER(mutexLocker, _lock);
-    builder.add(StaticStrings::LastValue, VPackValue(_lastValue));
   }
 
  protected:
+  /// @brief generate a new key value (internal)
   virtual uint64_t generateValue() = 0;
 
- private:
-  mutable arangodb::Mutex _lock;
-
-  uint64_t _lastValue;
+  /// @brief track a value (internal)
+  virtual void track(uint64_t value) = 0;
 };
 
 /// @brief traditional key generator for a single server
 class TraditionalKeyGeneratorSingle final : public TraditionalKeyGenerator {
  public:
   /// @brief create the generator
   explicit TraditionalKeyGeneratorSingle(bool allowUserKeys) 
-    : TraditionalKeyGenerator(allowUserKeys) {}
+    : TraditionalKeyGenerator(allowUserKeys), _lastValue(0) {
+    TRI_ASSERT(!ServerState::instance()->isCoordinator());
+  }
+
+  /// @brief build a VelocyPack representation of the generator in the builder
+  void toVelocyPack(arangodb::velocypack::Builder& builder) const override {
+    TraditionalKeyGenerator::toVelocyPack(builder);
+
+    // add our specific stuff 
+    MUTEX_LOCKER(mutexLocker, _lock);
+    builder.add(StaticStrings::LastValue, VPackValue(_lastValue));
+  }
 
  private:
-  /// @brief generate a key
+  /// @brief generate a key value (internal)
   uint64_t generateValue() override {
-    return TRI_NewTickServer();
+    uint64_t tick = TRI_NewTickServer();
+
+    if (ADB_UNLIKELY(tick == UINT64_MAX)) {
+      // out of keys
+      return 0;
+    }
+
+    // keep track of last assigned value, and make sure the value
+    // we hand out is always higher than it
+    {
+      MUTEX_LOCKER(mutexLocker, _lock);
+
+      if (ADB_UNLIKELY(_lastValue >= UINT64_MAX - 1ULL)) {
+        // oops, out of keys!
+        return 0;
+      }
+
+      if (tick <= _lastValue) {
+        tick = ++_lastValue;
+      } else {
+        _lastValue = tick;
+      }
+    }
+
+    return tick;
+  }
+
+  /// @brief track a key value (internal)
+  void track(ui
8000
nt64_t value) override {
+    MUTEX_LOCKER(mutexLocker, _lock);
+
+    if (value > _lastValue) {
+      // and update our last value
+      _lastValue = value;
+    }
   }
+
+ private:
+  mutable arangodb::Mutex _lock;
+
+  uint64_t _lastValue;
 };
 
 /// @brief traditional key generator for a coordinator
+/// please note that coordinator-based key generators are frequently
+/// created and discarded, so ctor & dtor need to be very efficient.
+/// additionally, do not put any state into this object, as for the
+/// same logical collection the ClusterInfo may create many different 
+/// temporary LogicalCollection objects one after the other, which
+/// will also discard the collection's particular KeyGenerator object!
 class TraditionalKeyGeneratorCluster final : public TraditionalKeyGenerator {
  public:
   /// @brief create the generator
   explicit TraditionalKeyGeneratorCluster(bool allowUserKeys)
-    : TraditionalKeyGenerator(allowUserKeys) {}
-
+    : TraditionalKeyGenerator(allowUserKeys) {
+    TRI_ASSERT(ServerState::instance()->isCoordinator());
+  }
+
  private:
-  /// @brief generate a key
+  /// @brief generate a key value (internal)
   uint64_t generateValue() override {
     ClusterInfo* ci = ClusterInfo::instance();
     return  ci->uniqid();
   }
+
+  /// @brief track a key value (internal)
+  void track(uint64_t /* value */) override {}
 };
 
 /// @brief base class for padded key generators
 class PaddedKeyGenerator : public KeyGenerator {
  public:
   /// @brief create the generator
   explicit PaddedKeyGenerator(bool allowUserKeys)
-    : KeyGenerator(allowUserKeys),
-      _lastValue(0) {}
+    : KeyGenerator(allowUserKeys) {}
 
   bool hasDynamicState() const override { return true; }
 
   /// @brief generate a key
   std::string generate() override {
     uint64_t tick = generateValue();
-
-    {
-      MUTEX_LOCKER(mutexLocker, _lock);
-
-      if (tick == UINT64_MAX || _lastValue >= UINT64_MAX - 1ULL) {
-        // oops, out of keys!
-        return std::string();
-      }
 
-      if (tick <= _lastValue) {
-        tick = ++_lastValue;
-      } else {
-        _lastValue = tick;
-      }
+    if (ADB_UNLIKELY(tick == 0)) {
+      // unlikely case we have run out of keys
+      // returning an empty string will trigger an error on the call site
+      return std::string();
     }
 
     return encode(tick);
@@ -287,26 +319,22 @@ class PaddedKeyGenerator : public KeyGenerator {
     // check the numeric key part
     uint64_t value = decode(p, length);
     if (value > 0) {
-      MUTEX_LOCKER(mutexLocker, _lock);
-
-      if (value > _lastValue) {
-        // and update our last value
-        _lastValue = value;
-      }
+      track(value);
     }
   }
 
   /// @brief build a VelocyPack representation of the generator in the builder
   void toVelocyPack(arangodb::velocypack::Builder& builder) const override {
     KeyGenerator::toVelocyPack(builder);
     builder.add("type", VPackValue("padded"));
-
-    MUTEX_LOCKER(mutexLocker, _lock);
-    builder.add(StaticStrings::LastValue, VPackValue(_lastValue));
   }
 
  protected:
+  /// @brief generate a key value (internal)
   virtual uint64_t generateValue() = 0;
+
+  /// @brief track a value (internal)
+  virtual void track(uint64_t value) = 0;
 
  private:
   uint64_t decode(char const* p, size_t length) {
@@ -360,40 +388,94 @@ class PaddedKeyGenerator : public KeyGenerator {
 
     return std::string(&buffer[0], sizeof(uint64_t) * 2);
   }
-
- private:
-  mutable arangodb::Mutex _lock;
-
-  uint64_t _lastValue;
 };
 
 /// @brief padded key generator for a single server
 class PaddedKeyGeneratorSingle final : public PaddedKeyGenerator {
  public:
   /// @brief create the generator
   explicit PaddedKeyGeneratorSingle(bool allowUserKeys) 
-    : PaddedKeyGenerator(allowUserKeys) {}
+    : PaddedKeyGenerator(allowUserKeys), _lastValue(0) {
+    TRI_ASSERT(!ServerState::instance()->isCoordinator());
+  }
+
+  /// @brief build a VelocyPack representation of the generator in the builder
+  void toVelocyPack(arangodb::velocypack::Builder& builder) const override {
+    PaddedKeyGenerator::toVelocyPack(builder);
+
+    // add our own specific values 
+    MUTEX_LOCKER(mutexLocker, _lock);
+    builder.add(StaticStrings::LastValue, VPackValue(_lastValue));
+  }
 
  private:
   /// @brief generate a key
   uint64_t generateValue() override {
-    return TRI_NewTickServer();
+    uint64_t tick = TRI_NewTickServer();
+
+    if (ADB_UNLIKELY(tick == UINT64_MAX)) {
+      // oops, out of keys!
+      return 0;
+    }
+
+    {
+      MUTEX_LOCKER(mutexLocker, _lock);
+
+      if (ADB_UNLIKELY(_lastValue >= UINT64_MAX - 1ULL)) {
+        // oops, out of keys!
+        return 0;
+      }
+
+      if (tick <= _lastValue) {
+        tick = ++_lastValue;
+      } else {
+        _lastValue = tick;
+      }
+    }
+
+    return tick;
+  }
+
+  /// @brief generate a key value (internal)
+  void track(uint64_t value) override {
+    MUTEX_LOCKER(mutexLocker, _lock);
+
+    if (value > _lastValue) {
+      // and update our last value
+      _lastValue = value;
+    }
   }
+
+ private:
+  mutable arangodb::Mutex _lock;
+
+  uint64_t _lastValue;
 };
 
 /// @brief padded key generator for a coordinator
+/// please note that coordinator-based key generators are frequently
+/// created and discarded, so ctor & dtor need to be very efficient.
+/// additionally, do not put any state into this object, as for the
+/// same logical collection the ClusterInfo may create many different 
+/// temporary LogicalCollection objects one after the other, which
+/// will also discard the collection's particular KeyGenerator object!
 class PaddedKeyGeneratorCluster final : public PaddedKeyGenerator {
  public:
   /// @brief create the generator
   explicit PaddedKeyGeneratorCluster(bool allowUserKeys)
-    : PaddedKeyGenerator(allowUserKeys) {}
+    : PaddedKeyGenerator(allowUserKeys) {
+    TRI_ASSERT(ServerState::instance()->isCoordinator());
+  }
 
  private:
-  /// @brief generate a key
+  /// @brief generate a key value (internal)
   uint64_t generateValue() override {
     ClusterInfo* ci = ClusterInfo::instance();
     return ci->uniqid();
   }
+
+  /// @brief generate a key value (internal)
+  void track(uint64_t /* value */) override {}
 };
 
 /// @brief auto-increment key generator - not usable in cluster

diff --git a/arangod/VocBase/KeyGenerator.h b/arangod/VocBase/KeyGenerator.h
@@ -35,6 +35,14 @@ class Builder;
 class Slice;
 }
 
+/// generic key generator interface
+///
+/// please note that coordinator-based key generators are frequently
+/// created and discarded, so ctor & dtor need to be very efficient.
+/// additionally, do not put any state into this object, as for the
+/// same logical collection the ClusterInfo may create many different 
+/// temporary LogicalCollection objects one after the other, which
+/// will also discard the collection's particular KeyGenerator object!
 class KeyGenerator {
   KeyGenerator(KeyGenerator const&) = delete;
   KeyGenerator& operator=(KeyGenerator const&) = delete;
@@ -55,6 +63,8 @@ class KeyGenerator {
   virtual bool hasDynamicState() const { return true; }
 
   /// @brief generate a key
+  /// if the returned string is empty, it means no proper key was
+  /// generated, and the caller must handle the situation
   virtual std::string generate() = 0;
 
   /// @brief validate a key

diff --git a/arangod/VocBase/LogicalCollection.h b/arangod/VocBase/LogicalCollection.h
@@ -75,6 +75,14 @@ class ChecksumResult : public Result {
   velocypack::Builder _builder;
 };
 
+/// please note that coordinator-based logical collections are frequently
+/// created and discarded, so ctor & dtor need to be as efficient as possible.
+/// additionally, do not put any volatile state into this object in the coordinator, 
+/// as the ClusterInfo may create many different temporary physical LogicalCollection 
+/// objects (one after the other) even for the same "logical" LogicalCollection.
+/// this which will also discard the collection's volatile state each time!
+/// all state of a LogicalCollection in the coordinator case needs to be derived
+/// from the JSON info in the agency's plan entry for the collection...
 class LogicalCollection : public LogicalDataSource {
   friend struct ::TRI_vocbase_t;
 
@@ -424,4 +432,4 @@ class LogicalCollection : public LogicalDataSource {
 
 }  // namespace arangodb
 
-#endif
+#endif