mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Till Toenshoff <toensh...@me.com>
Subject Re: Review Request 65939: Updated role endpoints for hierarchical accounting.
Date Thu, 05 Apr 2018 16:18:52 GMT


> On March 8, 2018, 12:20 a.m., Benjamin Mahler wrote:
> > src/master/http.cpp
> > Lines 3491 (patched)
> > <https://reviews.apache.org/r/65939/diff/1/?file=1972146#file1972146line3491>
> >
> >     Just to be sure, do we disallow double slashes? E.g. "a//b"? If so, then tokenize
and split provide the same functionality here, and I would be inclined to just use split to
avoid having to reason about what the implications of stripping empty tokens are (e.g. it
breaks the lookup logic?)

We do make sure that we got no double-slashes via https://github.com/apache/mesos/blob/master/src/common/roles.cpp#L71
so we could indeed use split here.


> On March 8, 2018, 12:20 a.m., Benjamin Mahler wrote:
> > src/master/http.cpp
> > Lines 3494-3513 (patched)
> > <https://reviews.apache.org/r/65939/diff/1/?file=1972146#file1972146line3494>
> >
> >     The way this is written the root 'Node' does not contain the sum of its children's
allocations, unlike all other nodes. This doesn't seem like an issue since the API does not
allow you to query for the root allocation. However, might as well make the root 'Node.resources'
contain the right information.

We could do this - so far I hesitated fearing that this duplicates the amount of invocations
on our resource arithmetics. Let's discuss value vs. price.


> On March 8, 2018, 12:20 a.m., Benjamin Mahler wrote:
> > src/master/http.cpp
> > Lines 3702-3710 (patched)
> > <https://reviews.apache.org/r/65939/diff/1/?file=1972146#file1972146line3707>
> >
> >     Rather than having to do this in the http code, can we have the Role struct
directly expose the aggregated information as well? I'd also like the Role struct to have
quota information, but that's for another patch :)

That is what my updated RR now proposes.


- Till


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65939/#review198755
-----------------------------------------------------------


On April 5, 2018, 4:15 p.m., Till Toenshoff wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65939/
> -----------------------------------------------------------
> 
> (Updated April 5, 2018, 4:15 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, Kapil Arya, Michael Park,
and Meng Zhu.
> 
> 
> Bugs: MESOS-8069
>     https://issues.apache.org/jira/browse/MESOS-8069
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> When listing role "a", resources allocated to role "a/b" are added to
> those allocated to role "a". These changes affect both, the "/roles"
> endpoint as well as the V1 HTTP Call "GET_ROLES".
> Adds dynamic hierarchical role tracking to the master.
> 
> 
> Diffs
> -----
> 
>   include/mesos/mesos.proto 676f0b090cad7ebf59eb32556f17ff8b5f247907 
>   include/mesos/v1/master/master.proto ddb28f96b2a3a439bb9a829995a9a3015f65ba43 
>   include/mesos/v1/mesos.proto 10d506517c9f098374ab0c8f4dcfda42e1896c95 
>   src/master/http.cpp 34c9023906eca94965acc994f20e888c1f47b962 
>   src/master/master.hpp 0d9620dd0c232dc1df83477e838eeb7313bf8828 
>   src/master/master.cpp 18382fa93fd0c59e641e00f2028ac1ae2e67c01c 
>   src/master/quota_handler.cpp 21bafd0064e9397f88185eaf687a58f85da94e2c 
>   src/master/weights_handler.cpp 1053652804a8fc6af31a3b8a9f632f836c897fa9 
>   src/tests/api_tests.cpp dd8e221d8fd1b2a241505345337897e4ee4a6347 
>   src/tests/role_tests.cpp a609ed27ef828ca82efc0d269669cda92e5f50a1 
> 
> 
> Diff: https://reviews.apache.org/r/65939/diff/2/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Till Toenshoff
> 
>


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