> On July 11, 2016, 8:23 p.m., Jie Yu wrote: > > src/slave/main.cpp, lines 175-179 > > <https://reviews.apache.org/r/46298/diff/2/?file=1349891#file1349891line175> > > > > Can we do that check in `add` function. `add` function supports an > > optional validate lambda to be passed in. > > Klaus Ma wrote: > @jie, thanks for the comments. Try to use validated lamba today, but > there some issues: > > 1. The `work_dir` is required without default value; if using validate > lamba, a "default" value is necessary: `add(T1* t1, name, alias, > defaultValue, lamba)` > 2. The default value is pass by const reference currently, we can not > pass some empty value to it, e.g. nullptr or 0 > > For the default value, I'm thinking to replace const reference with > `Option`; any suggestion? > > Jie Yu wrote: > Oh, I don't realize that we don't have validation support for required > field without default value. > > Can we introduce another `add` variant in flags that support the above > case? Is that difficult? > > Klaus Ma wrote: > Found a `add` function to add required field; this function need an > additional "optional alias" to avoid conflict. > > Jie Yu wrote: > we can introduce another overload for `add`, similar to this one: > > https://github.com/apache/mesos/blob/master/3rdparty/stout/include/stout/flags/flags.hpp#L182-L194 > > But, it accepts a validate function: > ``` > template <typename T1, typename F> > void add( > T1* t1, > const Name& name, > const std::string& help, > F validate) > { > add(t1, > name, > None(), > help, > static_cast<const T1*>(nullptr), > validate); > } > ``` > > Klaus Ma wrote: > @Jie, it will conflict with this one: > https://github.com/apache/mesos/blob/master/3rdparty/stout/include/stout/flags/flags.hpp#L172-L180 > . There's not enough information to distinguish `F validate` vs. `const T2& > t2`.
Aha, ic, now I get what you said about 'Option'. Yeah, can we make t2 an optional field here: https://github.com/apache/mesos/blob/master/3rdparty/stout/include/stout/flags/flags.hpp#L173 - Jie ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/46298/#review141756 ----------------------------------------------------------- On July 13, 2016, 5:45 a.m., Klaus Ma wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/46298/ > ----------------------------------------------------------- > > (Updated July 13, 2016, 5:45 a.m.) > > > Review request for mesos, Alexander Rukletsov and Jie Yu. > > > Bugs: MESOS-5123 > https://issues.apache.org/jira/browse/MESOS-5123 > > > Repository: mesos > > > Description > ------- > > Rejected relative path agent work_dir. > > > Diffs > ----- > > src/slave/flags.cpp 84dbb2db41bd9849ab1245969884675d9d16555b > > Diff: https://reviews.apache.org/r/46298/diff/ > > > Testing > ------- > > 1. make && make check > 2. e2e test: > > ``` > $ ./src/mesos-slave --work_dir=aa --master=aa > The required option `--work_dir` must be absolute path. > ``` > > > Thanks, > > Klaus Ma > >
