> On March 16, 2014, 9:28 p.m., Adam B wrote: > > Good start, Ashutosh. I think validating the sanity of the resources flag > > is vitally important, but I have my concerns about whether this is best > > done in slave/main.cpp, or in Containerizer or Slave itself. I think we > > should also discuss the desired behavior when an invalid resource flag > > value is provided. Should we exit without allowing the slave to start, or > > should we default to a sane value? > > Just a few questions and comments: > > - At the top of the review page, just below "Submitter: ASHUTOSH JAIN", > > please set "Branch: master" and "Bugs: MESOS-969". > > - As for testing, did you manually test that your changes fixed the problem > > you originally ran into? Can you write a unit test to represent that? > > ASHUTOSH JAIN wrote: > 1. Initially I did the check in containerizer.cpp and if flags were more > than the main memory then I set mem to total memory of system. But, this lead > to failure of DFR Allocation test and Resource Reservation tests so Bmahler > suggested to do this in main.cpp . > 2. I did a manual testing to verify the changes. I can try writing unit > test with some help from your side. Just point me to how to proceed > (preferably some example to how to write a test and where to write it) > > Ben Mahler wrote: > Adam, to your point of exiting vs defaulting: we do not want to modify > resources that are specified explicitly in the flags. That is, if an operator > asks the slave to allocate 8GB, and the machine has 6GB, it would be > preferable to let them know immediately rather than silently offer only 6GB. > Otherwise, this could prove very confusing for operators. > > However, we can use definitely use defaults if the resources are > unspecified.
* Agree with BenM. Exit if flags resources exceeds actual resources; use defaults if flags doesn't specify resources. - Testing: Why were your changes causing failures in DRFAllocation/ResourceReservation tests? Maybe we need to mock out os::memory() so that the tests use resources from a mocked system instead of whatever machine is running the test (which would have unpredictable available resources). - Adam ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/19263/#review37335 ----------------------------------------------------------- On March 17, 2014, 10:21 a.m., ASHUTOSH JAIN wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/19263/ > ----------------------------------------------------------- > > (Updated March 17, 2014, 10:21 a.m.) > > > Review request for mesos, Adam B, Adam Berry, and Ben Mahler. > > > Bugs: MESOS-969 > https://issues.apache.org/jira/browse/MESOS-969 > > > Repository: mesos-git > > > Description > ------- > > resources (mem) provided by user not being checked > > link to issue: https://issues.apache.org/jira/browse/MESOS-969 > > > Diffs > ----- > > src/slave/main.cpp 8c2b70c > > Diff: https://reviews.apache.org/r/19263/diff/ > > > Testing > ------- > > make check > > > Thanks, > > ASHUTOSH JAIN > >
