----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/19263/#review37335 -----------------------------------------------------------
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? src/slave/main.cpp <https://reviews.apache.org/r/19263/#comment68853> 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. src/slave/main.cpp <https://reviews.apache.org/r/19263/#comment68854> 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?). src/slave/main.cpp <https://reviews.apache.org/r/19263/#comment68855> Bytes mem = resources.mem().get(); src/slave/main.cpp <https://reviews.apache.org/r/19263/#comment68852> 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'. src/slave/main.cpp <https://reviews.apache.org/r/19263/#comment68850> Should we really exit here, or just default to mem_.total, perhaps minus some buffer, like what happens in Containerizer::resources()? Can we refactor/reuse that code? - Adam B On March 16, 2014, 8:23 a.m., ASHUTOSH JAIN wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/19263/ > ----------------------------------------------------------- > > (Updated March 16, 2014, 8:23 a.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 > >
