> On March 17, 2014, 4:28 a.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?
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) > On March 17, 2014, 4:28 a.m., Adam B wrote: > > src/slave/main.cpp, lines 138-140 > > <https://reviews.apache.org/r/19263/diff/2/?file=521464#file521464line138> > > > > The entire code block you added should be inside an if > > (flags.resources.isSome()) {}, since you shouldn't bother with anything if > > flags.resources isNone. I will modify this . > On March 17, 2014, 4:28 a.m., Adam B wrote: > > src/slave/main.cpp, lines 143-144 > > <https://reviews.apache.org/r/19263/diff/2/?file=521464#file521464line143> > > > > This should be an EXIT(1) if you cannot continue to launch the slave > > without this; or you could just LOG(ERROR) (preferable over cerr), and > > recover gracefully (set a default, or ignore?). I will modify this . > On March 17, 2014, 4:28 a.m., Adam B wrote: > > src/slave/main.cpp, lines 153-155 > > <https://reviews.apache.org/r/19263/diff/2/?file=521464#file521464line153> > > > > Bytes mem = resources.mem().get(); I will update this. > On March 17, 2014, 4:28 a.m., Adam B wrote: > > src/slave/main.cpp, lines 158-168 > > <https://reviews.apache.org/r/19263/diff/2/?file=521464#file521464line158> > > > > You're not actually "defaulting to DEFAULT_MEM" here, you're just > > warning then exiting. You need to modify the value of flags.resources so > > that the Containerizer and Slave created below will use your new values in > > 'flags.resources'. I will update this. - ASHUTOSH ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/19263/#review37335 ----------------------------------------------------------- On March 16, 2014, 3:23 p.m., ASHUTOSH JAIN wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/19263/ > ----------------------------------------------------------- > > (Updated March 16, 2014, 3:23 p.m.) > > > Review request for mesos, Adam B, Adam Berry, and Ben Mahler. > > > 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 > >
