mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Benjamin Bannier <benjamin.bann...@mesosphere.io>
Subject Re: Review Request 45018: MESOS-3481 Add const accessor to Master flags.
Date Mon, 21 Mar 2016 19:04:10 GMT

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




src/master/master.hpp (line 546)
<https://reviews.apache.org/r/45018/#comment187231>

    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.



src/tests/master_tests.cpp (line 38)
<https://reviews.apache.org/r/45018/#comment187237>

    Includes should remain sorted.



src/tests/master_tests.cpp (line 990)
<https://reviews.apache.org/r/45018/#comment187236>

    The flag you use isn't fake, but an existing one.



src/tests/master_tests.cpp (line 997)
<https://reviews.apache.org/r/45018/#comment187232>

    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?



src/tests/master_tests.cpp (line 1004)
<https://reviews.apache.org/r/45018/#comment187235>

    You should `await` the `Future` returned by `dispatch` here to ensure it is ready when
you `get` the `Flags`.


Will you update this patch later with changes making actual use of the functionality you add?

- Benjamin Bannier


On March 21, 2016, 3: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, 3: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