> On April 11, 2016, 2:53 p.m., Jojy Varghese wrote: > > src/cli/execute.cpp, line 196 > > <https://reviews.apache.org/r/46044/diff/1/?file=1339684#file1339684line196> > > > > I like the idea of simplifying the ctor. I am not too excited about the > > idea of moving everything to 'flag'. A `CommandScheduler` object should > > have some properties like `command`, `master`, `name`. Others like 'image' > > information should be moved to its own class/struct (say `ContainerInfo`). > > Just my 2 cents. > > Joseph Wu wrote: > I partially agree, but in this case, the `Flags` class is effectively the > class/struct you're asking for. It just happens to have everything packed > along with it :) > > Jojy Varghese wrote: > hmm i could extend that argument to get away with any type system or > class design and replace all types with a `flag` type. Good design would > allow translation between a user input (flag) to a CommandScheduler object. > Passing a user input to CommandScheduler does not seem right. I think this > was the reason the original author wanted input validation to happen before > the object was created. CommandScheduler should only accept a strongly typed > input in its ctor. I exceeded my 2 cents :|
I agree about input validation. All our new code puts flag validation directly inside the flags. i.e. https://github.com/apache/mesos/blob/master/src/master/flags.cpp#L420-L426 The existing error checking inside the `main` method should be moved into lambdas. At which point, passing `Flags` around would be equivalent to the previous explicit c-tor. - Joseph ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/46044/#review128265 ----------------------------------------------------------- On April 11, 2016, 12:01 p.m., Alexander Rukletsov wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/46044/ > ----------------------------------------------------------- > > (Updated April 11, 2016, 12:01 p.m.) > > > Review request for mesos, Anand Mazumdar and Joseph Wu. > > > Repository: mesos > > > Description > ------- > > Pass complete `flags` instance rather than each flag value separately > to `CommandScheduler` in mesos-execute for brevity. > > > Diffs > ----- > > src/cli/execute.cpp 763dd26c359d1dd92c6e0365e4808b673efb1f40 > > Diff: https://reviews.apache.org/r/46044/diff/ > > > Testing > ------- > > On Mac OS 10.10.4: > make check > > Additionally manually tested mesos-execute with both responsive and > unresponsive (https://github.com/rukletsov/unresponsive-process) tasks: > ./src/mesos-execute --master=127.0.0.1:5050 --name=test --command="sleep 10" > --env='{"GLOG_v": "2"}' --kill_after=2secs > ./src/mesos-execute --master=127.0.0.1:5050 --name=test > --command="/Users/alex/bin/unresponsive_process" --env='{"GLOG_v": "2"}' > --kill_after=2secs > > > Thanks, > > Alexander Rukletsov > >
