> On Dec. 8, 2016, 11:13 a.m., Benjamin Bannier wrote: > > src/slave/containerizer/mesos/io/switchboard_main.cpp, lines 71-76 > > <https://reviews.apache.org/r/54486/diff/1/?file=1578815#file1578815line71> > > > > Using sentinel values in the parameter domain as default values seems > > wrong to me. If you want to detect explicitly given flags you should just > > use an `Option<T>` for the parameter. This verification then becomes the > > semantically clearer > > > > if (flags.stdin_to_fd.isNone() || > > ... > > > > With that no default value in the `add()` calls is required. > > > > You should probably also adjust the error message to something like > > `Missing requireed parameters` and output `flags.usage()` after that. > > Jie Yu wrote: > Sounds good. > > Benjamin Bannier wrote: > @jieyu: I see this was committed without addressing any of these issues > (or at least leaving a comment), and also without updating the RR. Is there > any plan to fix these? > > Jie Yu wrote: > Yes. I'll fix those. thanks!
Should this be closed now? - Kevin ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/54486/#review158513 ----------------------------------------------------------- On Dec. 7, 2016, 5:06 p.m., Kevin Klues wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/54486/ > ----------------------------------------------------------- > > (Updated Dec. 7, 2016, 5:06 p.m.) > > > Review request for mesos, Benjamin Bannier and Jie Yu. > > > Bugs: MESOS-6726 > https://issues.apache.org/jira/browse/MESOS-6726 > > > Repository: mesos > > > Description > ------- > > Added default values for all required IOSwitchboardServerFlags. > > > Diffs > ----- > > src/slave/containerizer/mesos/io/switchboard.hpp > f691b182d4029a15bbb3b1c158b176d43f265cc1 > src/slave/containerizer/mesos/io/switchboard_main.cpp > 4a9ed22ca63f0aa55ebbf7b85404a1b168a8e34a > > Diff: https://reviews.apache.org/r/54486/diff/ > > > Testing > ------- > > GTEST_FILTER="" make -j check > GTEST_FILTER="*IOSwitchboard*" src/mesos-tests > > > Thanks, > > Kevin Klues > >
