----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/61473/#review182605 -----------------------------------------------------------
Some of the comments below were made before I started to feel that we are probably doing too many conversions to justify storing these tasks in TASK_UNREACHABLE. Perhaps we can just store them in `Framework.unreachableTasks` but in TASK_LOST state? It's possible that we can add another map `BoundedHashMap<TaskID, process::Owned<Task>> unreachableNonPartitionAwareTasks;` for these tasks but it's clunky in the sense that you have to clarify that `unreachableTasks` is only for partition aware tasks but in fact all of these tasks belong to the same framework which is either parition aware or not, however with the possibility of changing capability... so it's probably easier to describe things if we just put all of them in `unreachableTasks` and simply say that "if the framework is not partition-aware, the tasks stored in `unreachableTasks` may be in `TASK_LOST`". If we do that, then some of the comments below don't apply any more but I am keep them just for posterity (some styling issues etc). src/master/http.cpp Lines 342 (patched) <https://reviews.apache.org/r/61473/#comment258567> One empty line above. src/master/http.cpp Lines 3203-3206 (original), 3215-3227 (patched) <https://reviews.apache.org/r/61473/#comment258574> Should we only loop through the once? And in order to try not to duplicate similar lines, consider the following? ``` foreachvalue (const Owned<Task>& _task, framework->unreachableTasks) { Owned<Task> task = _task; if (framework->capabilities.partitionAware) { task = ... } frameworkTaskSummaries[frameworkId].count(*task.get()); slaveTaskSummaries[task->slave_id()].count(*task.get()); } ``` src/master/http.cpp Lines 4017-4027 (original), 4038-4063 (patched) <https://reviews.apache.org/r/61473/#comment258575> Similar to above, don't duplicate lines. src/master/http.cpp Lines 4167-4175 (original), 4203-4221 (patched) <https://reviews.apache.org/r/61473/#comment258576> Ditto. src/master/master.hpp Lines 2403-2408 (patched) <https://reviews.apache.org/r/61473/#comment258578> Can we hold off creating a helper for this? IMO this helper is doing too little and not so much of an "abstraction", i.e., what the method does it not perfectly captured by the name of the mehod. Inlining 1) making a copy & 2) setting the state is not too much redudancy than calling this method. src/master/master.hpp Lines 2769-2771 (original), 2773-2775 (patched) <https://reviews.apache.org/r/61473/#comment258600> Fix the comment. src/master/master.cpp Lines 6398-6414 (original), 6389-6405 (patched) <https://reviews.apache.org/r/61473/#comment258580> Fix the comments. src/master/master.cpp Lines 6422 (patched) <https://reviews.apache.org/r/61473/#comment258579> `erase` can handle the `!contains` case. src/master/master.cpp Lines 6477-6480 (original), 6464-6467 (patched) <https://reviews.apache.org/r/61473/#comment258584> Fix the comment. src/master/master.cpp Line 7137 (original), 7112 (patched) <https://reviews.apache.org/r/61473/#comment258586> If our code stops making such distinction, I don't think the comment should continue to make such distinction. src/master/master.cpp Lines 7132-7137 (patched) <https://reviews.apache.org/r/61473/#comment258590> Do this only when we actually send an update i.e., `framework->connected()`? src/master/master.cpp Lines 8928-8931 (original), 8907-8910 (patched) <https://reviews.apache.org/r/61473/#comment258599> So for this update event we are going to send TASK_UNREACHABLE instead TASK_LOST, which is unintended? - Jiang Yan Xu On Aug. 10, 2017, 9:07 a.m., Megha Sharma wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/61473/ > ----------------------------------------------------------- > > (Updated Aug. 10, 2017, 9:07 a.m.) > > > Review request for mesos, Vinod Kone and Jiang Yan Xu. > > > Bugs: MESOS-7215 > https://issues.apache.org/jira/browse/MESOS-7215 > > > Repository: mesos > > > Description > ------- > > Master will not kill the tasks for non-Partition aware frameworks > when an unreachable agent re-registers with the master. > Master used to send a ShutdownFrameworkMessages to the agent > to kill the tasks from non partition aware frameworks including the > ones that are still registered which was problematic because the offer > from this agent could still go to the same framework which could then > launch new tasks. The agent would then receive tasks of the same > framework and ignore them because it thinks the framework is shutting > down. The framework is not shutting down of course, so from the master > and the scheduler’s perspective the task is pending in STAGING forever > until the next agent reregistration, which could happen much later. > This commit fixes the problem by not shutting down the non-partition > aware frameworks on such an agent. > > > Diffs > ----- > > src/master/http.cpp 959091c8ec03b6ac7bcb5d21b04d2f7d5aff7d54 > src/master/master.hpp b802fd153a10f6012cea381f153c28cc78cae995 > src/master/master.cpp 7f38a5e21884546d4b4c866ca5918db779af8f99 > src/tests/partition_tests.cpp 62a84f797201ccd18b71490949e3130d2b9c3668 > > > Diff: https://reviews.apache.org/r/61473/diff/3/ > > > Testing > ------- > > make check > > > Thanks, > > Megha Sharma > >
