> On May 17, 2017, 9:44 a.m., Adam B wrote: > > src/common/http.cpp > > Lines 510-511 (patched) > > <https://reviews.apache.org/r/58095/diff/7/?file=1714046#file1714046line510> > > > > 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.
It is not the only namespace to be in a different namespace, `void json(JSON::ObjectWriter* writer, const Task& task);` is part of the [`internal`](https://github.com/apache/mesos/blob/master/src/common/http.hpp#L125) namespace. The issue here is that most of the info's object are part only of the `mesos` namespace, but quota is part of the [`mesos::quota` namespace](https://github.com/apache/mesos/blob/master/include/mesos/quota/quota.proto#L21). > On May 17, 2017, 9:44 a.m., Adam B wrote: > > src/common/http.cpp > > Lines 518-520 (patched) > > <https://reviews.apache.org/r/58095/diff/7/?file=1714046#file1714046line518> > > > > Were we printing the principal before? Seems like unnecessary info > > that'll just clog up the output. Yes we do, this is an example of the response from mesos running in my machine: ```json { "roles": [ { "frameworks": [ "5a8720f2-69a3-43b4-b889-1905ac9460df-0000" ], "name": "space-odyssey", "quota": { "guarantee": { "cpus": 2.0, "disk": 250.0, "gpus": 0, "mem": 250.0 }, "principal": "hal-9000", "role": "space-odyssey" }, "resources": { "cpus": 1.0, "disk": 32.0, "gpus": 0, "mem": 128.0, "ports": "[31002-31003]" }, "weight": 1.0 } ] } ``` > On May 17, 2017, 9:44 a.m., Adam B wrote: > > src/master/http.cpp > > Lines 410 (patched) > > <https://reviews.apache.org/r/58095/diff/7/?file=1714047#file1714047line410> > > > > 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? the file [role_tests.cpp](https://github.com/apache/mesos/blob/master/src/tests/role_tests.cpp) contains tests which validates the output of the endpoint. > On May 17, 2017, 9:44 a.m., Adam B wrote: > > src/master/http.cpp > > Lines 412 (patched) > > <https://reviews.apache.org/r/58095/diff/7/?file=1714047#file1714047line412> > > > > Why a `vector<int>`? Wouldn't it be a `vector<string>` since it's a > > vector of `FrameworkID`s? I think at this point we just want to generate the empty string `[]`, so either should create the proper json empty array. Probably using `vector<string>` would be less missleading indeed. - Alexander ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/58095/#review175208 ----------------------------------------------------------- On May 10, 2017, 6:32 p.m., Jay Guo wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/58095/ > ----------------------------------------------------------- > > (Updated May 10, 2017, 6:32 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/7/ > > > Testing > ------- > > no functional changes > make check > > > Thanks, > > Jay Guo > >
