mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Adam B <a...@mesosphere.io>
Subject Re: Review Request 58095: Refactored `Master::Http::roles` to use `jsonify`.
Date Wed, 17 May 2017 07:44:48 GMT

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


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