----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/59320/#review176399 -----------------------------------------------------------
src/master/master.cpp Lines 6018-6025 (original), 6018-6025 (patched) <https://reviews.apache.org/r/59320/#comment249773> Hm.. what does slaveWasRemoved mean? Is it equivalen to the agent being marked as unreachable? Or is being removed something beyond being marked as unreachable (what it sounds like)? I had a hard time grasping the code below when it was referring to "unreachable" but the variable is called "removed". src/master/master.cpp Lines 6027-6043 (original), 6027-6043 (patched) <https://reviews.apache.org/r/59320/#comment249772> Hm.. it seems like we could maybe make this code easier to read. Currently, the loop for dealing with frameworks is down below and the loop for dealing with tasks is up here. However, it seems to me that these are highly related, and it would be easier to reason about if it were consolidated into a loop over the framework and then a loop over the tasks belonging to that framework. That would also eliminate the need for `partitionAwareFrameworks`. E.g. ``` foreach (const FrameworkInfo& framework, frameworks): if completed: shut down the framework, drop the tasks continue if agent was unreachable/removed and framework is not partition aware: shut down the framework, drop the tasks continue for each task in the framework: recoveredTasks.push_back(task) remove from unreachable tasks ``` src/master/master.cpp Lines 6058 (patched) <https://reviews.apache.org/r/59320/#comment249766> "it" is referring to the task? Threw me off initially, since it sounds like "remove the framework" given the previous setence. s/it/the task/ ? src/tests/partition_tests.cpp Lines 2387 (patched) <https://reviews.apache.org/r/59320/#comment249765> Hm.. why disabled on windows? Do you know why or is this just copied? For example, it's not clear to me why the above partition test isn't disabled but the below one is. - Benjamin Mahler On May 16, 2017, 8:19 p.m., Neil Conway wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/59320/ > ----------------------------------------------------------- > > (Updated May 16, 2017, 8:19 p.m.) > > > Review request for mesos and Benjamin Mahler. > > > Repository: mesos > > > Description > ------- > > Add an explanatory comment and a test case for a particular case in the > master's logic for handling agent re-registration. > > > Diffs > ----- > > src/master/master.cpp 4e7a161f431624bd78d3e9032eb8587687149cad > src/tests/partition_tests.cpp 4ff428564d1fa6cb96e6f8ec8edc331da88a3eb6 > > > Diff: https://reviews.apache.org/r/59320/diff/2/ > > > Testing > ------- > > `make check` > > Checked that if you add `CHECK_NOTNULL(framework)` to the master code (circa > the newly added comment), the incorrect `CHECK` is triggered by this test > case (but not by any existing test cases). > > > Thanks, > > Neil Conway > >
