----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/57110/#review167777 -----------------------------------------------------------
src/master/master.hpp Lines 2260-2263 (patched) <https://reviews.apache.org/r/57110/#comment239753> This case is just one possibility, it's also possible that the framework removes the role with tasks running still, or an agent that still needed to register but wasn't partitioned reports back. So maybe something more general? ``` // It's possible that we're not tracking the task's role for // this framework if the role is absent from the framework's // set of roles. In this case, we track the role's allocation // for this framework. ``` src/master/master.hpp Lines 2263 (patched) <https://reviews.apache.org/r/57110/#comment239752> This last sentence seems a bit redundant? src/master/master.hpp Lines 2320-2324 (original), 2355-2375 (patched) <https://reviews.apache.org/r/57110/#comment239755> Can we just call `recoverResources(task)` here? src/master/master.hpp Lines 2448-2451 (patched) <https://reviews.apache.org/r/57110/#comment239754> Ditto here. src/master/master.hpp Lines 2526 (patched) <https://reviews.apache.org/r/57110/#comment239756> At this point, you have based it on `info` instead of `source`, right? Maybe that's a little clearer since we just adjusted `info`? src/master/master.cpp Lines 2637-2638 (original), 2637-2638 (patched) <https://reviews.apache.org/r/57110/#comment239757> Do you need the quotes here? This ends up coming out like this: ``` Could not update FrameworkInfo of framework '<ID> (hostname) at 10.10.10.10:5050': ... ``` Whereas I think your intention was to quote the ID. Seems like this is the responsibility of the output operator for `Framework&`. src/master/master.cpp Lines 2951-2952 (original), 2941-2942 (patched) <https://reviews.apache.org/r/57110/#comment239758> Oh I see it was copied from here.. we should fix the quoting here as well. src/master/master.cpp Lines 3301 (patched) <https://reviews.apache.org/r/57110/#comment239742> s/std::// Also I think it fits on one line at that point, like the others below. src/master/master.cpp Lines 3335-3337 (patched) <https://reviews.apache.org/r/57110/#comment239741> Should we use .at() for these three lines that intend const access into the map? src/master/master.cpp Lines 5992-5993 (patched) <https://reviews.apache.org/r/57110/#comment239759> Per our offline discussion, looks like this belongs in one of the previous reviews that addresses the addition of the FrameworkInfo into the message? Seems like we need to send it always now that it includes the info. src/master/master.cpp Lines 6085-6091 (patched) <https://reviews.apache.org/r/57110/#comment239761> This could be simplified to: ``` const set<string> removedRoles = oldRoles - newRoles; ``` If we were to add the `-` operator to the existing operators in stout/set.hpp. src/master/master.cpp Lines 6116-6118 (patched) <https://reviews.apache.org/r/57110/#comment239763> And we'll expose this via metrics right? Maybe just say that we'll still account for the allocation to the role for the framework? src/master/master.cpp Lines 6124-6130 (patched) <https://reviews.apache.org/r/57110/#comment239762> Ditto here, could be: ``` const set<string> addedRoles = newRoles - oldRoles; ``` src/master/master.cpp Lines 6133-6136 (patched) <https://reviews.apache.org/r/57110/#comment239764> How about a newline between the comment blocks for readability? ``` // We only add the framework to the role if it doesn't already exist. // // NOTE: It's possible that the framework already exists in the role since // we don't remove the framework from the role until there are no resources // being used by the framework within that role. ``` Ditto on the other notes. src/master/master.cpp Line 7453 (original), 7568 (patched) <https://reviews.apache.org/r/57110/#comment239743> Ditto here, this can just be: ``` foreach (const string& role, framework->roles) { // ... } ``` src/master/master.cpp Lines 7453-7460 (original), 7568-7576 (patched) <https://reviews.apache.org/r/57110/#comment239749> Ditto here, I guess this case needs to be called from the constructor? Wonder if we need a `Framework::initialize`? src/master/master.cpp Line 7549 (original), 7656 (patched) <https://reviews.apache.org/r/57110/#comment239738> Should this be composing the message? ``` return Error("Failed to update framework: " + updateFramework_.error()); ``` src/master/master.cpp Lines 7892-7893 (original), 7992-7993 (patched) <https://reviews.apache.org/r/57110/#comment239739> No need to call the helper since we already maintain the set within the Framework struct. This can just be: ``` foreach (const string& role, framework->roles) { removeFrameworkFromRole(framework, role); } ``` src/master/master.cpp Lines 7892-7894 (original), 7992-7994 (patched) <https://reviews.apache.org/r/57110/#comment239747> Currently, these functions are sometimes called from the Framework struct and sometimes from the Master, so the responsibilities between the master and the framework seem a little unclear, and potentially error prone. Is it possible to place all of the calls into the role management into the framework struct? From our conversation offline, it sounds like we'll need to introduce a function like `Framework::complete` so that we can avoid calling `removeFrameworkFromRole` from the Framework destructor (which doesn't work since we keep completed frameworks). - Benjamin Mahler On Feb. 27, 2017, 10:19 p.m., Michael Park wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/57110/ > ----------------------------------------------------------- > > (Updated Feb. 27, 2017, 10:19 p.m.) > > > Review request for mesos and Benjamin Mahler. > > > Bugs: MESOS-6627 > https://issues.apache.org/jira/browse/MESOS-6627 > > > Repository: mesos > > > Description > ------- > > See summary. > > > Diffs > ----- > > src/master/master.hpp 764adb18ddf09b62529c5c96f8e4dfaf7803483e > src/master/master.cpp 2ef836536784d5fda23c80dc2304239b176a8942 > > > Diff: https://reviews.apache.org/r/57110/diff/1/ > > > Testing > ------- > > > Thanks, > > Michael Park > >
