mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jay Guo <guojian...@cn.ibm.com>
Subject Re: Review Request 45018: MESOS-3481 Add const accessor to Master flags.
Date Tue, 22 Mar 2016 02:52:54 GMT


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


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