mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Alexander Rojas <alexan...@mesosphere.io>
Subject Re: Review Request 58095: Refactored `Master::Http::roles` to use `jsonify`.
Date Wed, 17 May 2017 08:35:34 GMT


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


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message