Repository: mesos Updated Branches: refs/heads/master 3b70d417a -> 316b433db
Remove unnecessary hashmap lookups. In a number of places, we tested whether the slaves hashmap contains the desired element before indexing it. It is safe to just index it and check for a NULL result, which saves us some hashing and lookups. Review: https://reviews.apache.org/r/58304/ Project: http://git-wip-us.apache.org/repos/asf/mesos/repo Commit: http://git-wip-us.apache.org/repos/asf/mesos/commit/316b433d Tree: http://git-wip-us.apache.org/repos/asf/mesos/tree/316b433d Diff: http://git-wip-us.apache.org/repos/asf/mesos/diff/316b433d Branch: refs/heads/master Commit: 316b433db108ab60f156089ebac6d7c9fc2ddc8b Parents: 3b70d41 Author: James Peach <[email protected]> Authored: Wed Apr 19 17:11:14 2017 -0700 Committer: Neil Conway <[email protected]> Committed: Wed Apr 19 17:23:09 2017 -0700 ---------------------------------------------------------------------- src/master/master.cpp | 68 +++++++++++++++++++--------------------------- 1 file changed, 28 insertions(+), 40 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/mesos/blob/316b433d/src/master/master.cpp ---------------------------------------------------------------------- diff --git a/src/master/master.cpp b/src/master/master.cpp index 52de2f9..d1cdc35 100644 --- a/src/master/master.cpp +++ b/src/master/master.cpp @@ -1311,10 +1311,7 @@ void Master::exited(const UPID& pid) } } - if (slaves.registered.contains(pid)) { - Slave* slave = slaves.registered.get(pid); - CHECK_NOTNULL(slave); - + if (Slave* slave = slaves.registered.get(pid)) { LOG(INFO) << "Agent " << *slave << " disconnected"; if (slave->connected) { @@ -5272,7 +5269,9 @@ void Master::executorMessage( // The slave should (re-)register with the master before // forwarding executor messages. - if (!slaves.registered.contains(slaveId)) { + Slave* slave = slaves.registered.get(slaveId); + + if (slave == nullptr) { LOG(WARNING) << "Ignoring executor message" << " from executor '" << executorId << "'" << " of framework " << frameworkId @@ -5281,9 +5280,6 @@ void Master::executorMessage( return; } - Slave* slave = slaves.registered.get(slaveId); - CHECK_NOTNULL(slave); - Framework* framework = getFramework(frameworkId); if (framework == nullptr) { @@ -5403,10 +5399,7 @@ void Master::registerSlave( } // Check if this slave is already registered (because it retries). - if (slaves.registered.contains(from)) { - Slave* slave = slaves.registered.get(from); - CHECK_NOTNULL(slave); - + if (Slave* slave = slaves.registered.get(from)) { if (!slave->connected) { // The slave was previously disconnected but it is now trying // to register as a new slave. This could happen if the slave @@ -5592,9 +5585,7 @@ void Master::reregisterSlave( return; } - Slave* slave = slaves.registered.get(slaveInfo.id()); - - if (slave != nullptr) { + if (Slave* slave = slaves.registered.get(slaveInfo.id())) { CHECK(!slaves.recovered.contains(slaveInfo.id())); // NOTE: This handles the case where a slave tries to @@ -6103,15 +6094,15 @@ void Master::updateSlave( return; } - if (!slaves.registered.contains(slaveId)) { + Slave* slave = slaves.registered.get(slaveId); + + if (slave == nullptr) { LOG(WARNING) << "Ignoring update of agent with total oversubscribed resources " << oversubscribedResources << " on unknown agent " << slaveId; return; } - Slave* slave = CHECK_NOTNULL(slaves.registered.get(slaveId)); - LOG(INFO) << "Received update of agent " << *slave << " with total" << " oversubscribed resources " << oversubscribedResources; @@ -6170,7 +6161,7 @@ void Master::updateUnavailability( // The slave should be registered if it is in the machines mapping. CHECK(slaves.registered.contains(slaveId)); - Slave* slave = CHECK_NOTNULL(slaves.registered.get(slaveId)); + Slave* slave = slaves.registered.get(slaveId); if (unavailability.isSome()) { // TODO(jmlvanre): Add stream operator for unavailability. @@ -6367,7 +6358,9 @@ void Master::exitedExecutor( return; } - if (!slaves.registered.contains(slaveId)) { + Slave* slave = slaves.registered.get(slaveId); + + if (slave == nullptr) { LOG(WARNING) << "Ignoring exited executor '" << executorId << "' of framework " << frameworkId << " on unknown agent " << slaveId; @@ -6377,9 +6370,6 @@ void Master::exitedExecutor( // Only update master's internal data structures here for proper // accounting. The TASK_LOST updates are handled by the slave. - Slave* slave = slaves.registered.get(slaveId); - CHECK_NOTNULL(slave); - if (!slave->hasExecutor(frameworkId, executorId)) { LOG(WARNING) << "Ignoring unknown exited executor '" << executorId << "' of framework " << frameworkId @@ -6425,25 +6415,25 @@ void Master::shutdown( // TODO(vinod): Add a metric for executor shutdowns. - if (!slaves.registered.contains(shutdown.slave_id())) { - LOG(WARNING) << "Unable to shutdown executor '" << shutdown.executor_id() - << "' of framework " << framework->id() - << " of unknown agent " << shutdown.slave_id(); - return; - } - const SlaveID& slaveId = shutdown.slave_id(); const ExecutorID& executorId = shutdown.executor_id(); + const FrameworkID& frameworkId = framework->id(); Slave* slave = slaves.registered.get(slaveId); - CHECK_NOTNULL(slave); + + if (slave == nullptr) { + LOG(WARNING) << "Unable to shutdown executor '" << executorId + << "' of framework " << frameworkId + << " of unknown agent " << slaveId; + return; + } LOG(INFO) << "Processing SHUTDOWN call for executor '" << executorId << "' of framework " << *framework << " on agent " << slaveId; ShutdownExecutorMessage message; message.mutable_executor_id()->CopyFrom(executorId); - message.mutable_framework_id()->CopyFrom(framework->id()); + message.mutable_framework_id()->CopyFrom(frameworkId); send(slave->pid, message); } @@ -6973,7 +6963,9 @@ void Master::offer( foreachpair (const SlaveID& slaveId, const Resources& offered, resources.at(role)) { - if (!slaves.registered.contains(slaveId)) { + Slave* slave = slaves.registered.get(slaveId); + + if (slave == nullptr) { LOG(WARNING) << "Master returning resources offered to framework " << *framework << " because agent " << slaveId << " is not valid"; @@ -6982,9 +6974,6 @@ void Master::offer( continue; } - Slave* slave = slaves.registered.get(slaveId); - CHECK_NOTNULL(slave); - // This could happen if the allocator dispatched 'Master::offer' before // the slave was deactivated in the allocator. if (!slave->active) { @@ -7120,16 +7109,15 @@ void Master::inverseOffer( foreachpair (const SlaveID& slaveId, const UnavailableResources& unavailableResources, resources) { - if (!slaves.registered.contains(slaveId)) { + Slave* slave = slaves.registered.get(slaveId); + + if (slave == nullptr) { LOG(INFO) << "Master ignoring inverse offers to framework " << *framework << " because agent " << slaveId << " is not valid"; continue; } - Slave* slave = slaves.registered.get(slaveId); - CHECK_NOTNULL(slave); - // This could happen if the allocator dispatched 'Master::inverseOffer' // before the slave was deactivated in the allocator. if (!slave->active) {
