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