mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Anindya Sinha <anindya_si...@apple.com>
Subject Re: Review Request 47082: LostSlaveMessage should be sent to affected frameworks only.
Date Thu, 26 May 2016 23:56:20 GMT


> 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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message