Improved Registrar performance by using a hashset on SlaveIDs. Review: https://reviews.apache.org/r/19762
Project: http://git-wip-us.apache.org/repos/asf/mesos/repo Commit: http://git-wip-us.apache.org/repos/asf/mesos/commit/033d788f Tree: http://git-wip-us.apache.org/repos/asf/mesos/tree/033d788f Diff: http://git-wip-us.apache.org/repos/asf/mesos/diff/033d788f Branch: refs/heads/master Commit: 033d788fda059345e715269f2fb9b2bea622be1d Parents: 21f823d Author: Jiang Yan Xu <[email protected]> Authored: Thu Mar 27 11:27:40 2014 -0700 Committer: Jiang Yan Xu <[email protected]> Committed: Thu Apr 17 16:20:47 2014 -0700 ---------------------------------------------------------------------- src/master/master.hpp | 36 ++++++++++++++++++++++-------------- src/master/registrar.cpp | 14 ++++++++++++-- src/master/registrar.hpp | 41 ++++++++++++++++++++++++++++++++++------- 3 files changed, 68 insertions(+), 23 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/mesos/blob/033d788f/src/master/master.hpp ---------------------------------------------------------------------- diff --git a/src/master/master.hpp b/src/master/master.hpp index e4c7862..f567a43 100644 --- a/src/master/master.hpp +++ b/src/master/master.hpp @@ -785,21 +785,23 @@ public: } protected: - virtual Try<bool> perform(Registry* registry, bool strict) + virtual Try<bool> perform( + Registry* registry, + hashset<SlaveID>* slaveIDs, + bool strict) { // Check and see if this slave already exists. - foreach (const Registry::Slave& slave, registry->slaves().slaves()) { - if (slave.info().id() == info.id()) { - if (strict) { - return Error("Slave already admitted"); - } else { - return false; // No mutation. - } + if (slaveIDs->contains(info.id())) { + if (strict) { + return Error("Slave already admitted"); + } else { + return false; // No mutation. } } Registry::Slave* slave = registry->mutable_slaves()->add_slaves(); slave->mutable_info()->CopyFrom(info); + slaveIDs->insert(info.id()); return true; // Mutation. } @@ -818,12 +820,13 @@ public: } protected: - virtual Try<bool> perform(Registry* registry, bool strict) + virtual Try<bool> perform( + Registry* registry, + hashset<SlaveID>* slaveIDs, + bool strict) { - foreach (const Registry::Slave& slave, registry->slaves().slaves()) { - if (slave.info().id() == info.id()) { - return false; // No mutation. - } + if (slaveIDs->contains(info.id())) { + return false; // No mutation. } if (strict) { @@ -831,6 +834,7 @@ protected: } else { Registry::Slave* slave = registry->mutable_slaves()->add_slaves(); slave->mutable_info()->CopyFrom(info); + slaveIDs->insert(info.id()); return true; // Mutation. } } @@ -850,12 +854,16 @@ public: } protected: - virtual Try<bool> perform(Registry* registry, bool strict) + virtual Try<bool> perform( + Registry* registry, + hashset<SlaveID>* slaveIDs, + bool strict) { for (int i = 0; i < registry->slaves().slaves().size(); i++) { const Registry::Slave& slave = registry->slaves().slaves(i); if (slave.info().id() == info.id()) { registry->mutable_slaves()->mutable_slaves()->DeleteSubrange(i, 1); + slaveIDs->erase(info.id()); return true; // Mutation. } } http://git-wip-us.apache.org/repos/asf/mesos/blob/033d788f/src/master/registrar.cpp ---------------------------------------------------------------------- diff --git a/src/master/registrar.cpp b/src/master/registrar.cpp index 9c0537d..38040bd 100644 --- a/src/master/registrar.cpp +++ b/src/master/registrar.cpp @@ -105,7 +105,10 @@ private: explicit Recover(const MasterInfo& _info) : info(_info) {} protected: - virtual Try<bool> perform(Registry* registry, bool strict) + virtual Try<bool> perform( + Registry* registry, + hashset<SlaveID>* slaveIDs, + bool strict) { registry->mutable_master()->mutable_info()->CopyFrom(info); return true; // Mutation. @@ -312,11 +315,18 @@ void RegistrarProcess::update() CHECK_SOME(variable); + // Create a snapshot of the current registry. Registry registry = variable.get().get(); + // Create the 'slaveIDs' accumulator. + hashset<SlaveID> slaveIDs; + foreach (const Registry::Slave& slave, registry.slaves().slaves()) { + slaveIDs.insert(slave.info().id()); + } + foreach (Owned<Operation> operation, operations) { // No need to process the result of the operation. - (*operation)(®istry, flags.registry_strict); + (*operation)(®istry, &slaveIDs, flags.registry_strict); } // TODO(benh): Add a timeout so we don't wait forever. http://git-wip-us.apache.org/repos/asf/mesos/blob/033d788f/src/master/registrar.hpp ---------------------------------------------------------------------- diff --git a/src/master/registrar.hpp b/src/master/registrar.hpp index 0d659c5..6bc78c4 100644 --- a/src/master/registrar.hpp +++ b/src/master/registrar.hpp @@ -21,6 +21,8 @@ #include <mesos/mesos.hpp> +#include <stout/hashset.hpp> + #include <process/future.hpp> #include <process/owned.hpp> @@ -38,17 +40,25 @@ class RegistrarProcess; // Defines an abstraction for operations that can be applied on the // Registry. +// TODO(xujyan): Make Operation generic so that we can apply them +// against a generic "batch operation applier" abstraction, see TODO +// below for more details. class Operation : public process::Promise<bool> { public: Operation() : success(false) {} - - // Attempts to invoke the operation on 't'. - // Returns whether the operation mutates 't', or an error if the - // operation cannot be applied successfully. - Try<bool> operator () (Registry* registry, bool strict) + virtual ~Operation() {} + + // Attempts to invoke the operation on 'registry' (and the + // accumulators, in this case 'slaveIDs'). + // Returns whether the operation mutates 'registry', or an error if + // the operation cannot be applied successfully. + Try<bool> operator () ( + Registry* registry, + hashset<SlaveID>* slaveIDs, + bool strict) { - const Try<bool>& result = perform(registry, strict); + const Try<bool>& result = perform(registry, slaveIDs, strict); success = !result.isError(); @@ -59,13 +69,30 @@ public: bool set() { return process::Promise<bool>::set(success); } protected: - virtual Try<bool> perform(Registry* registry, bool strict) = 0; + virtual Try<bool> perform( + Registry* registry, + hashset<SlaveID>* slaveIDs, + bool strict) = 0; private: bool success; }; +// TODO(xujyan): Add a generic abstraction for applying batches of +// operations against State Variables. The Registrar and other +// components could leverage this. This abstraction would be +// templatized to take the type, along with any accumulators: +// template <typename T, +// typename X = None, +// typename Y = None, +// typename Z = None> +// T: the data type that the batch operations can be applied on. +// X, Y, Z: zero to 3 generic accumulators that facilitate the batch +// of operations. +// This allows us to reuse the idea of "doing batches of operations" +// on other potential new state variables (i.e. repair state, offer +// reservations, etc). class Registrar { public:
