Re: Review Request 45018: MESOS-3481 Add const accessor to Master flags.

[Jay Guo]

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

(Updated March 22, 2016, 3:34 a.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 (updated)
-

  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

Re: Review Request 45018: MESOS-3481 Add const accessor to Master flags.

[Jay Guo]

> On March 21, 2016, 7:04 p.m., Benjamin Bannier wrote:
> > src/master/master.hpp, line 546
> > 
> >
> > 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
> > 
> >
> > Includes should remain sorted.

OK


> On March 21, 2016, 7:04 p.m., Benjamin Bannier wrote:
> > src/tests/master_tests.cpp, line 990
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > 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
> 
>

Re: Review Request 45018: MESOS-3481 Add const accessor to Master flags.

[Benjamin Bannier]

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




src/master/master.hpp (line 546)


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)


Includes should remain sorted.



src/tests/master_tests.cpp (line 990)


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



src/tests/master_tests.cpp (line 997)


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)


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

Re: Review Request 45018: MESOS-3481 Add const accessor to Master flags.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [45018]

Passed command: export OS='ubuntu:14.04' CONFIGURATION='--verbose' 
COMPILER='gcc' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker_build.sh

- Mesos ReviewBot


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

Review Request 45018: MESOS-3481 Add const accessor to Master flags.

[Jay Guo]

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

Review request for mesos, Joris Van Remoortere and Joseph Wu.


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