> On March 21, 2016, 7:04 p.m., Benjamin Bannier wrote:
> > src/master/master.hpp, line 546
> > <https://reviews.apache.org/r/45018/diff/1/?file=1304894#file1304894line546>
> >
> >     Two things:
> >     
> >     (1) I think a more fitting name here might be `flags()` (cf `info()` 
> > just above).
> >     
> >     (2) I would have expected a getter to return a const ref; this always 
> > performs a copy.
> >     
> >     I would also have expected this function to be `const` itself; was the 
> > only reason you didn't do that for the dispatch in your test below? If yes, 
> > I believe this should rather be fixed in libprocess instead of us adding 
> > more suprising API.

(1) flags() would be a duplicate member of flags
(2) dispatch doesn't like reference. It yields compilation error:
`../../3rdparty/libprocess/include/process/future.hpp:147:10: error: 
'operator->' declared as a pointer to a reference of type 'const 
mesos::internal::master::Flags &'`

And yes, we don't have a dispatch version that handles const function. Should 
we add it to libprocess?


> On March 21, 2016, 7:04 p.m., Benjamin Bannier wrote:
> > src/tests/master_tests.cpp, line 38
> > <https://reviews.apache.org/r/45018/diff/1/?file=1304895#file1304895line38>
> >
> >     Includes should remain sorted.

OK


> On March 21, 2016, 7:04 p.m., Benjamin Bannier wrote:
> > src/tests/master_tests.cpp, line 990
> > <https://reviews.apache.org/r/45018/diff/1/?file=1304895#file1304895line990>
> >
> >     The flag you use isn't fake, but an existing one.

Sure, we should refine the text


> On March 21, 2016, 7:04 p.m., Benjamin Bannier wrote:
> > src/tests/master_tests.cpp, line 997
> > <https://reviews.apache.org/r/45018/diff/1/?file=1304895#file1304895line997>
> >
> >     This directory is never cleaned up (on neither success nor failure 
> > paths).
> >     
> >     Even if it was cleaned up, it might fail if the `cwd` is not writable. 
> > Could you pick a less intrusive approach for this test?

OK, we will evaluate other attributes instead of mkdir


> On March 21, 2016, 7:04 p.m., Benjamin Bannier wrote:
> > src/tests/master_tests.cpp, line 1004
> > <https://reviews.apache.org/r/45018/diff/1/?file=1304895#file1304895line1004>
> >
> >     You should `await` the `Future` returned by `dispatch` here to ensure 
> > it is ready when you `get` the `Flags`.

OK


On March 21, 2016, 7:04 p.m., Jay Guo wrote:
> > Will you update this patch later with changes making actual use of the 
> > functionality you add?

Thanks for your review! We will update the patch accordingly.


- Jay


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45018/#review124620
-----------------------------------------------------------


On March 21, 2016, 2:05 p.m., Jay Guo wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45018/
> -----------------------------------------------------------
> 
> (Updated March 21, 2016, 2:05 p.m.)
> 
> 
> Review request for mesos, Joris Van Remoortere and Joseph Wu.
> 
> 
> Bugs: MESOS-3481
>     https://issues.apache.org/jira/browse/MESOS-3481
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> MESOS-3481 Add const accessor to Master flags.
> 
> 
> Diffs
> -----
> 
>   src/master/master.hpp 124d43931a5c8a00ee0aaa604feb1761795209f2 
>   src/tests/master_tests.cpp d34ba0bdd71efd261850d8c205c16cecb701ac7c 
> 
> Diff: https://reviews.apache.org/r/45018/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> This getter is used to retrieve the flags from master. Currently, if one 
> wants to access master flags in the test, master flag has to be explicitly 
> created and then passed to `CreateMasterFlags(flags)`.
> 
> story description: https://issues.apache.org/jira/browse/MESOS-3481
> 
> 
> Thanks,
> 
> Jay Guo
> 
>

Reply via email to