----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/58095/#review175208 -----------------------------------------------------------
Re: "Testing Done": Can you point to the unit test that proves we haven't change the output of /roles? I'd love to see the before/after json from a manual test on a master with quota and weights set for a couple of roles. src/common/http.cpp Lines 510-511 (patched) <https://reviews.apache.org/r/58095/#comment248615> Why is this the only `json()` that nees to be in its type's namespace. Does QuotaInfo really need to be in the `quota` namespace? Sounds like stuttering to me. Not your problem, I guess, but I'd suggest a follow-up JIRA to remove the redundant namespacing there. `quota::Info` or `::QuotaInfo` is descriptive enough. src/common/http.cpp Lines 518-520 (patched) <https://reviews.apache.org/r/58095/#comment248616> Were we printing the principal before? Seems like unnecessary info that'll just clog up the output. src/master/http.cpp Lines 410 (patched) <https://reviews.apache.org/r/58095/#comment248636> Not for backwards compatibility, I think, but for roles that only exist in weight/quota definitions, without resources reserved or frameworks registered. Do we have a test case that exercises this path? src/master/http.cpp Lines 412 (patched) <https://reviews.apache.org/r/58095/#comment248638> Why a `vector<int>`? Wouldn't it be a `vector<string>` since it's a vector of `FrameworkID`s? src/master/http.cpp Lines 3492-3493 (original), 3497-3498 (patched) <https://reviews.apache.org/r/58095/#comment248630> Fits on one line. No need to wrap. Also, why rename `filteredRoles` to `activeRoles`? That variable is the result of the `_roles(principal)` authorization filtering. Maybe `authorizedRoles` if you're going to rename. Or call them `roleNames` since it's just the string representation and not the full `Role` object. - Adam B On May 10, 2017, 9:32 a.m., Jay Guo wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/58095/ > ----------------------------------------------------------- > > (Updated May 10, 2017, 9:32 a.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/7/ > > > Testing > ------- > > no functional changes > make check > > > Thanks, > > Jay Guo > >
