> On May 25, 2016, 6:27 p.m., Jiang Yan Xu wrote: > > src/master/master.cpp, line 6551 > > <https://reviews.apache.org/r/47082/diff/1/?file=1375592#file1375592line6551> > > > > I don't feel that this boolean is necessary, we can easily check if a > > framework is added by checking if it's in the set (O(1) if hashset), right?
For a std::set, we needed this bool. I think we can avoid this if we use a hashset. > On May 25, 2016, 6:27 p.m., Jiang Yan Xu wrote: > > src/master/master.cpp, lines 6576-6579 > > <https://reviews.apache.org/r/47082/diff/1/?file=1375592#file1375592line6576> > > > > From `slave->checkpointedResource.reservations()` we can get the list > > of roles and from `activeRoles` we can get the list of frameworks for each > > role. Moved this to use activeRoles. > On May 25, 2016, 6:27 p.m., Jiang Yan Xu wrote: > > src/tests/master_authorization_tests.cpp, lines 338-339 > > <https://reviews.apache.org/r/47082/diff/1/?file=1375593#file1375593line338> > > > > If we don't expect it to be call, we should do > > > > ``` > > EXPECT_CALL(sched, slaveLost(&driver, _)) > > .Times(0); > > ``` > > > > Here and elsewhere. > > > > But here I think we should fix this to have the framework receive the > > slave lost messsage. See the comment below. Reinstated the original version, since we expect SlaveLostMessage to be sent for pending tasks. > On May 25, 2016, 6:27 p.m., Jiang Yan Xu wrote: > > src/tests/master_authorization_tests.cpp, lines 346-348 > > <https://reviews.apache.org/r/47082/diff/1/?file=1375593#file1375593line346> > > > > So if the task is stuck in the `pendingTasks` a slave lost message is > > not sent but later a TASK_LOST is sent with reason > > `REASON_SLAVE_REMOVED`... We should handle this case the same way as the > > other cases where we do send slave lost IMO. > > > > We probably need to add a `pendingTasks` hashmap in the slave as well > > and check against that map during `removeSlave()`, thoughts? Good catch. Yes, added a slave->pendingTasks and use this to send out SlaveLostMessage to frameworks with pending tasks. - Anindya ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/47082/#review134535 ----------------------------------------------------------- On May 26, 2016, 11:56 p.m., Anindya Sinha wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/47082/ > ----------------------------------------------------------- > > (Updated May 26, 2016, 11:56 p.m.) > > > Review request for mesos and Jiang Yan Xu. > > > Bugs: MESOS-5143 > https://issues.apache.org/jira/browse/MESOS-5143 > > > Repository: mesos > > > Description > ------- > > When a slave is removed, master sends a LostSlaveMessage to affected > frameworks only (instead of all registered frameworks). An affected > framework is a framework which satisfied one or more conditions of > the following: > > 1. There are running tasks on this slave belonging to the framework. > 2. There are pending tasks on this slave belonging to the framework. > 3. Reserved resources on the slave have a matching role with the > role of the framework. > 4. There are pending offers or pending inverse offers from this slave > for the framework. > > > Diffs > ----- > > src/master/master.hpp 1a875c32eddfb6d884e3d0dda7f5716ee53966c3 > src/master/master.cpp 0005a29caabcc6a3776037cf86a2b12660e6377b > src/tests/master_tests.cpp 34be015aa314a7574e9065efb7b1bb8e1570c5b7 > > Diff: https://reviews.apache.org/r/47082/diff/ > > > Testing > ------- > > All existing and modified tests passed. > > > Thanks, > > Anindya Sinha > >
