mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Vinod Kone" <vinodk...@gmail.com>
Subject Re: Review Request 34910: Added task validation for task using revocable resources while its executor does not.
Date Wed, 10 Jun 2015 19:10:10 GMT


> On June 3, 2015, 6:08 p.m., Ben Mahler wrote:
> > Can we make the test a unit test? Looks like we could pull up '`validateResources`'
to make this unit-testable? Chatting with Jie, we should probably place it in an `'internal::task'`
namespace towards the bottom of the header, since it's only for testing purposes and we want
to make it clear which validation functions are expected to be used by the master.

done.


> On June 3, 2015, 6:08 p.m., Ben Mahler wrote:
> > src/master/validation.cpp, lines 321-326
> > <https://reviews.apache.org/r/34910/diff/1/?file=975981#file975981line321>
> >
> >     Are we otherwise allowing mixing within a resource? Looking at the existing
cpushare.cpp logic, it seems that we have a binary decision:
> >     
> >       (1) If any of the cpus are revocable, *all* cpus are treated revocable for
isolation purposes.
> >       (2) Otherwise, non-revocable.
> >     
> >     Is the idea here that the mixing that (1) allows enables a framework to reduce
the "risk" of revocation?
> >     
> >     Maybe a TODO here and/or on the cpushare code since it's not that difficult
to change the cpushare implementation to allow transitioning between revocable and non-revocable
after the container is launched?

per the latest discussion, we are not allowing mixing of resources (revocable and non-revocable)
for any given type (e.g., cpus).


> On June 3, 2015, 6:08 p.m., Ben Mahler wrote:
> > src/tests/master_validation_tests.cpp, line 1105
> > <https://reviews.apache.org/r/34910/diff/1/?file=975982#file975982line1105>
> >
> >     its :)

N/A.


> On June 3, 2015, 6:08 p.m., Ben Mahler wrote:
> > src/tests/master_validation_tests.cpp, lines 1136-1137
> > <https://reviews.apache.org/r/34910/diff/1/?file=975982#file975982line1136>
> >
> >     I find it nice to reduce "jaggedness" on comments, e.g.:
> >     
> >     ```
> >       // Create a task that uses revocable resources
> >       // while it's executor does not.
> >     ```

I think most of our code just wraps the comments at 70.


> On June 3, 2015, 6:08 p.m., Ben Mahler wrote:
> > src/tests/master_validation_tests.cpp, lines 1104-1105
> > <https://reviews.apache.org/r/34910/diff/1/?file=975982#file975982line1104>
> >
> >     From this comment, I expected this test to be testing that a task with revocable
resources is allowed when the executor has revocable resources, but it's only testing the
opposite condition.
> >     
> >     The unit test would make it easier to test both cases, yes?

added tests for both cases.


- Vinod


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34910/#review86449
-----------------------------------------------------------


On June 1, 2015, 11:11 p.m., Vinod Kone wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34910/
> -----------------------------------------------------------
> 
> (Updated June 1, 2015, 11:11 p.m.)
> 
> 
> Review request for mesos, Ben Mahler, Ian Downes, Jie Yu, and Joris Van Remoortere.
> 
> 
> Bugs: MESOS-2753
>     https://issues.apache.org/jira/browse/MESOS-2753
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Enforces the invariant that a task cannot use revocable resources unless its executor
does.
> 
> 
> Diffs
> -----
> 
>   src/master/validation.cpp 1793b0e16fc36ead147880ee06cda40b23fa0962 
>   src/tests/master_validation_tests.cpp dc9e91e120c2af9e72013557730f6a2fbb5b00fe 
> 
> Diff: https://reviews.apache.org/r/34910/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Vinod Kone
> 
>


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