mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jie Yu <yujie....@gmail.com>
Subject Re: Review Request 46298: Rejected relative path agent work_dir.
Date Thu, 14 Jul 2016 00:21:35 GMT


> 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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message