You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Jay Guo <gu...@gmail.com> on 2017/05/08 07:41:35 UTC
Re: Review Request 58095: Refactored `Master::Http::roles` to use
`jsonify`.
-----------------------------------------------------------
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.
Changes
-------
address comments.
Summary (updated)
-----------------
Refactored `Master::Http::roles` to use `jsonify`.
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 (updated)
-------
Refactored `Master::Http::roles` to use `jsonify`.
Diffs (updated)
-----
src/common/http.hpp 93d6088e97c2384f9f6d26e010a501abf2deb43e
src/common/http.cpp 167dce2b9a2d3b68a1df5b4079f701482d34db28
src/master/http.cpp e2590a17044ac019b24a24629428d4ec8adc0c31
Diff: https://reviews.apache.org/r/58095/diff/5/
Changes: https://reviews.apache.org/r/58095/diff/4-5/
Testing
-------
no functional changes
make check
Thanks,
Jay Guo
Re: Review Request 58095: Refactored `Master::Http::roles` to use
`jsonify`.
Posted by Alexander Rojas <al...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58095/#review174444
-----------------------------------------------------------
I really like the shape this is taking. Just a really minor issue and it will be ready!
src/master/http.cpp
Lines 3499-3509 (original), 3501-3511 (patched)
<https://reviews.apache.org/r/58095/#comment247618>
Could you revert these lines to the style:
```c++
Option<type> variable = None();
if (master->attribute.contains(name)) {
variable = master->attribute.at(name);
}
```
also, note that we don't use the curly braces `{}` for constructors in order to keep consistency (although I must admit there may be some that pass through the review process).
Feel free to use them for initialization lists.
- Alexander Rojas
On May 8, 2017, 9:41 a.m., Jay Guo wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58095/
> -----------------------------------------------------------
>
> (Updated May 8, 2017, 9:41 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/5/
>
>
> Testing
> -------
>
> no functional changes
> make check
>
>
> Thanks,
>
> Jay Guo
>
>
Re: Review Request 58095: Refactored `Master::Http::roles` to use
`jsonify`.
Posted by Alexander Rojas <al...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58095/#review174453
-----------------------------------------------------------
src/master/http.cpp
Lines 3489-3497 (original), 3491-3499 (patched)
<https://reviews.apache.org/r/58095/#comment247624>
you can do something like:
```c++
Master *master = this->master;
return _roles(principal)
.then(defer(master->self(),
[master, request](...) {
...
```
and that avoids having to capture `this`.
- Alexander Rojas
On May 8, 2017, 9:41 a.m., Jay Guo wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58095/
> -----------------------------------------------------------
>
> (Updated May 8, 2017, 9:41 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/5/
>
>
> Testing
> -------
>
> no functional changes
> make check
>
>
> Thanks,
>
> Jay Guo
>
>
Re: Review Request 58095: Refactored `Master::Http::roles` to use
`jsonify`.
Posted by Jay Guo <gu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58095/
-----------------------------------------------------------
(Updated June 22, 2017, 2:01 a.m.)
Review request for mesos, Adam B, Alexander Rojas, Benjamin Mahler, and Michael Park.
Changes
-------
minor tweak
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 (updated)
-----
src/common/http.hpp 93d6088e97c2384f9f6d26e010a501abf2deb43e
src/common/http.cpp 2f7718cbc2e449b4f7c89754e8f84eac2f3c35b6
src/master/http.cpp 4dd43fd7c3fb986f4eed78bce574b6d3af156b67
Diff: https://reviews.apache.org/r/58095/diff/9/
Changes: https://reviews.apache.org/r/58095/diff/8-9/
Testing
-------
no functional changes
make check
Thanks,
Jay Guo
Re: Review Request 58095: Refactored `Master::Http::roles` to use
`jsonify`.
Posted by Jay Guo <gu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58095/
-----------------------------------------------------------
(Updated June 22, 2017, 1:57 a.m.)
Review request for mesos, Adam B, Alexander Rojas, Benjamin Mahler, and Michael Park.
Changes
-------
rebase and address comments
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 (updated)
-----
src/common/http.hpp 93d6088e97c2384f9f6d26e010a501abf2deb43e
src/common/http.cpp 2f7718cbc2e449b4f7c89754e8f84eac2f3c35b6
src/master/http.cpp 4dd43fd7c3fb986f4eed78bce574b6d3af156b67
Diff: https://reviews.apache.org/r/58095/diff/8/
Changes: https://reviews.apache.org/r/58095/diff/7-8/
Testing
-------
no functional changes
make check
Thanks,
Jay Guo
Re: Review Request 58095: Refactored `Master::Http::roles` to use
`jsonify`.
Posted by Alexander Rojas <al...@mesosphere.io>.
> 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
>
>
Re: Review Request 58095: Refactored `Master::Http::roles` to use
`jsonify`.
Posted by Adam B <ad...@mesosphere.io>.
-----------------------------------------------------------
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
>
>
Re: Review Request 58095: Refactored `Master::Http::roles` to use
`jsonify`.
Posted by Alexander Rojas <al...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58095/#review174540
-----------------------------------------------------------
Ship it!
Ship It!
- Alexander Rojas
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
>
>
Re: Review Request 58095: Refactored `Master::Http::roles` to use
`jsonify`.
Posted by Jay Guo <gu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58095/
-----------------------------------------------------------
(Updated May 11, 2017, 12:32 a.m.)
Review request for mesos, Adam B, Alexander Rojas, Benjamin Mahler, and Michael Park.
Changes
-------
rebase
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 (updated)
-----
src/common/http.hpp 93d6088e97c2384f9f6d26e010a501abf2deb43e
src/common/http.cpp 167dce2b9a2d3b68a1df5b4079f701482d34db28
src/master/http.cpp e2590a17044ac019b24a24629428d4ec8adc0c31
Diff: https://reviews.apache.org/r/58095/diff/7/
Changes: https://reviews.apache.org/r/58095/diff/6-7/
Testing
-------
no functional changes
make check
Thanks,
Jay Guo
Re: Review Request 58095: Refactored `Master::Http::roles` to use
`jsonify`.
Posted by Jay Guo <gu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58095/
-----------------------------------------------------------
(Updated May 10, 2017, 9:42 p.m.)
Review request for mesos, Adam B, Alexander Rojas, Benjamin Mahler, and Michael Park.
Changes
-------
address comments.
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 (updated)
-----
src/common/http.hpp 93d6088e97c2384f9f6d26e010a501abf2deb43e
src/common/http.cpp 167dce2b9a2d3b68a1df5b4079f701482d34db28
src/master/http.cpp e2590a17044ac019b24a24629428d4ec8adc0c31
Diff: https://reviews.apache.org/r/58095/diff/6/
Changes: https://reviews.apache.org/r/58095/diff/5-6/
Testing
-------
no functional changes
make check
Thanks,
Jay Guo