> On April 27, 2017, 10:58 p.m., Alexander Rojas wrote: > > src/master/http.cpp > > Line 3401 (original), 3403 (patched) > > <https://reviews.apache.org/r/58095/diff/4/?file=1693998#file1693998line3403> > > > > your changes here make this function non safe thread. Notice that the > > original would run in the context of the `MasterProcess` thread, while this > > will do so in the caller's thread, causing a possible race condition.
Reverted to keep using `_roles()`. > On April 27, 2017, 10:58 p.m., Alexander Rojas wrote: > > src/master/http.cpp > > Lines 3481-3482 (original), 3456-3457 (patched) > > <https://reviews.apache.org/r/58095/diff/4/?file=1693998#file1693998line3495> > > > > Not yours, but I feel the formatting of this lambda is really off. The > > body of the lambda starts a couple of columns earlier than declaration. Other indents in this file are off too, e.g. `state()`, `frameworks()` > On April 27, 2017, 10:58 p.m., Alexander Rojas wrote: > > src/master/http.cpp > > Line 3504 (original), 3505 (patched) > > <https://reviews.apache.org/r/58095/diff/4/?file=1693998#file1693998line3548> > > > > Is ist really necesary to add empty fields? Without this, `RoleTest.EndpointNoFrameworks` would be broken. I suppose we need empty fields for backwards compatibility? > On April 27, 2017, 10:58 p.m., Alexander Rojas wrote: > > src/master/master.hpp > > Lines 1399-1401 (original), 1399-1405 (patched) > > <https://reviews.apache.org/r/58095/diff/4/?file=1693999#file1693999line1399> > > > > How about rewording this to: > > > > ```c++ > > // List of active roles, i.e. roles which are > > // explicitly white listed, plus roles with one or > > // more registered frameworks, plus all roles with a > > // non default weight or quota. > > ``` > > > > This gives you a clear understanding of what the method returns without > > describing how you do it (which is non interesting for API documentation). > > Also, the first paragraph doesn't really says anything useful. Left original method untouched. - Jay ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/58095/#review173186 ----------------------------------------------------------- On May 8, 2017, 3:41 p.m., Jay Guo wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/58095/ > ----------------------------------------------------------- > > (Updated May 8, 2017, 3:41 p.m.) > > > Review request for mesos, Adam B, Alexander Rojas, Benjamin Mahler, and > Michael Park. > > > Bugs: MESOS-4732 and MESOS-7260 > https://issues.apache.org/jira/browse/MESOS-4732 > https://issues.apache.org/jira/browse/MESOS-7260 > > > Repository: mesos > > > Description > ------- > > Refactored `Master::Http::roles` to use `jsonify`. > > > Diffs > ----- > > src/common/http.hpp 93d6088e97c2384f9f6d26e010a501abf2deb43e > src/common/http.cpp 167dce2b9a2d3b68a1df5b4079f701482d34db28 > src/master/http.cpp e2590a17044ac019b24a24629428d4ec8adc0c31 > > > Diff: https://reviews.apache.org/r/58095/diff/5/ > > > Testing > ------- > > no functional changes > make check > > > Thanks, > > Jay Guo > >
