From reviews-return-54007-apmail-mesos-reviews-archive=mesos.apache.org@mesos.apache.org Wed Jan 18 15:15:07 2017 Return-Path: X-Original-To: apmail-mesos-reviews-archive@minotaur.apache.org Delivered-To: apmail-mesos-reviews-archive@minotaur.apache.org Received: from mail.apache.org (hermes.apache.org [140.211.11.3]) by minotaur.apache.org (Postfix) with SMTP id 2841E190AB for ; Wed, 18 Jan 2017 15:15:07 +0000 (UTC) Received: (qmail 34311 invoked by uid 500); 18 Jan 2017 15:15:07 -0000 Delivered-To: apmail-mesos-reviews-archive@mesos.apache.org Received: (qmail 34274 invoked by uid 500); 18 Jan 2017 15:15:07 -0000 Mailing-List: contact reviews-help@mesos.apache.org; run by ezmlm Precedence: bulk List-Help: List-Unsubscribe: List-Post: List-Id: Reply-To: reviews@mesos.apache.org Delivered-To: mailing list reviews@mesos.apache.org Received: (qmail 34260 invoked by uid 99); 18 Jan 2017 15:15:06 -0000 Received: from reviews-vm.apache.org (HELO reviews.apache.org) (140.211.11.40) by apache.org (qpsmtpd/0.29) with ESMTP; Wed, 18 Jan 2017 15:15:06 +0000 Received: from reviews.apache.org (localhost [127.0.0.1]) by reviews.apache.org (Postfix) with ESMTP id F39C5315617; Wed, 18 Jan 2017 15:15:05 +0000 (UTC) Content-Type: multipart/alternative; boundary="===============1715046092631408249==" MIME-Version: 1.0 Subject: Re: Review Request 54183: Improved management of unreachable and completed tasks in master. From: Vinod Kone To: Vinod Kone Cc: Mesos ReviewBot , Neil Conway , mesos Date: Wed, 18 Jan 2017 15:15:05 -0000 Message-ID: <20170118151505.28414.96617@reviews.apache.org> X-ReviewBoard-URL: https://reviews.apache.org/ Auto-Submitted: auto-generated Sender: Vinod Kone X-ReviewGroup: mesos X-Auto-Response-Suppress: DR, RN, OOF, AutoReply X-ReviewRequest-URL: https://reviews.apache.org/r/54183/ X-Sender: Vinod Kone References: <20170103222534.13569.45280@reviews.apache.org> In-Reply-To: <20170103222534.13569.45280@reviews.apache.org> Reply-To: Vinod Kone X-ReviewRequest-Repository: mesos --===============1715046092631408249== MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit > On Jan. 3, 2017, 10:25 p.m., Vinod Kone wrote: > > src/master/master.cpp, line 5507 > > > > > > can we inline this? > > Neil Conway wrote: > We could, but personally I find it more readable to make it a separate function. The logic is a bit involved here (since we need to check two different data structures due to backward compatibility requirements), and there's already a lot going on in `Master::_reregisterSlave`. > > Vinod Kone wrote: > In general, we tend to factor out something into a function if it is reusable and generic, not to reduce the number of lines someone has to read in a function. In fact the argument was to inline in such cases because it will avoid the need to context switch. > > Both `findPartitonAwareFrameworks` and `filterPartitionTasks` seem very specific to the `_reregisterSlave` method, hence the suggestion. > > For example, instead of looping through all the agent's tasks 3 times to figure out which tasks to add to `Slave*`, it's probably easier to do it in one inline loop here? > > Neil Conway wrote: > My goal isn't to reduce the # of lines to read the function, but to raise the level of abstraction a little bit. For example, `findPartitonAwareFrameworks` has to check _two_ data structures, depending on both agent version and whether the master has failed over. That is quite strange and takes a bit of thought to validate. With a separate function, the reader of `_reregisterSlave` doesn't need to think about those details if they don't want to -- instead they can just think "okay, we find all the partition-aware frameworks on the agent and then ..." > > I'm fine with looping over the agent's tasks three times, because each of the three things we want to do is logically independent -- it takes a bit of thought to validate whether it is actually correct to combine them into a single loop (because each loop iteration would access a collection that is modified by the loop itself. It would actually be okay, but IMO current approach is safer). This is the code I had in mind. Let me know what you think: ``` // Add recovered frameworks into a hashset for easy lookup. hashset frameworks_; foreach (const FrameworkInfo& framework, frameworks) { frameworks_.put(framework.id(), framework); } vector tasks_; foreach (const Task& task, tasks) { const FrameworkInfo& frameworkInfo = frameworks_[task.framework_id()]; Framework* framework = getFramework(framework.id()); if (protobuf::frameworkHasCapability(frameworkInfo, PARTITION_AWARE)) { // Add the task if it is partition aware. tasks_.push_back(task); // Remove the partition aware task from `unreachableTasks`. if (framework != null) { framework->unreachableTasks.erase(task.task_id()); } } else if (slaves.removed.get(slaveInfo.id()).isSome()) { // Add a non-partion task iff the master failed over; tasks_.push_back(task); } } Slave* slave = new Slave( this, slaveInfo, pid, machineId, version, Clock::now(), checkpointedResources, executorInfos, tasks_); } ``` - Vinod ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/54183/#review160019 ----------------------------------------------------------- On Jan. 17, 2017, 6:36 p.m., Neil Conway wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/54183/ > ----------------------------------------------------------- > > (Updated Jan. 17, 2017, 6:36 p.m.) > > > Review request for mesos and Vinod Kone. > > > Bugs: MESOS-6619 > https://issues.apache.org/jira/browse/MESOS-6619 > > > Repository: mesos > > > Description > ------- > > Before partition-awareness, when an agent failed health checks, the > master removed the agent from the registry, marked all of its tasks > TASK_LOST, and moved them to the `completedTasks` list in the master's > memory. Although "lost" tasks might still be running, partitioned agents > would only be allowed to re-register if the master failed over, in which > case the `completedTasks` map would be emptied. > > When partition-awareness was introduced, we initially followed the same > scheme, with the only difference that partition-aware tasks are marked > TASK_UNREACHABLE, not TASK_LOST. > > This scheme has a few shortcomings. First, partition-aware tasks might > resume running when the partitioned agent re-registers. Second, we > re-added non-partition aware tasks when the agent re-registered but then > marked them completed when the framework is shutdown, resulting in two > entries in `completedTasks`. > > This commit introduces a separate bounded map, `unreachableTasks`. These > tasks are reported separately via the HTTP endpoints, because they have > different semantics (unlike completed tasks, unreachable tasks can > resume running). The size of this map is limited by a new master flag, > `--max_unreachable_tasks_per_framework`. This commit also changes the > master to omit re-adding non-partition-aware tasks on re-registering > agents (unless the master has failed over): those tasks will shortly be > shutdown anyway. > > Finally, this commit fixes a minor bug in the previous code: the > previous coding neglected to shutdown non-partition-aware frameworks > running on pre-1.0 Mesos agents that re-register with the master after > a network partition. > > > Diffs > ----- > > docs/configuration.md 2113d06f58dddc0a28ae1241a24096266fe39801 > include/mesos/master/master.proto 03203c8ddd05876339b1c56789b7bb140499d49d > include/mesos/v1/master/master.proto f8edf39a68752c8601cece345f52bce8b9a8a68b > src/master/constants.hpp 900b69460de9280f30b16b86925e436054e080ca > src/master/flags.hpp 6a17b763dc76daa10073394f416b049e97a44238 > src/master/flags.cpp 124bfbca6be9d5d60597b9a1793bd807202da8ec > src/master/http.cpp a44621f39cb059e654a56f57f75b38947f3a4230 > src/master/master.hpp 44f4fecb1fbe8bebf830990a59a5462338e6e004 > src/master/master.cpp b863ff6e93931c3d1ee056248084c7f44caf2fd9 > src/tests/partition_tests.cpp 72013d1bfee275c6f3cb90173f0c408d55e0bc5d > > Diff: https://reviews.apache.org/r/54183/diff/ > > > Testing > ------- > > `make check` > > > Thanks, > > Neil Conway > > --===============1715046092631408249==--