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