mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Benjamin Bannier <benjamin.bann...@mesosphere.io>
Subject Re: Review Request 43880: Added allocator metrics for total and allocated scalar resources.
Date Fri, 04 Mar 2016 16:02:14 GMT


> On March 4, 2016, 2:05 p.m., Alexander Rukletsov wrote:
> > src/master/allocator/mesos/metrics.hpp, line 27
> > <https://reviews.apache.org/r/43880/diff/14/?file=1279199#file1279199line27>
> >
> >     Kill this line.

As if it never was there now.


> On March 4, 2016, 2:05 p.m., Alexander Rukletsov wrote:
> > src/master/allocator/mesos/metrics.cpp, lines 61-63
> > <https://reviews.apache.org/r/43880/diff/14/?file=1279200#file1279200line61>
> >
> >     Does it make sense to store the pointer to `HierarchicalAllocatorProcess` in
the `Metrics` instance and remove the first parameter here? The value of this approach will
be more evident when we will have more helpers later.

Yes, that makes sense. This puts some more restrictions on how an allocator `Metrics` can
be used. For now I made the class e.g., non-copyable and added a `NOTE` on the lifetime dependency
with the `HierarchicalAllocatorProcess`.


> On March 4, 2016, 2:05 p.m., Alexander Rukletsov wrote:
> > src/tests/hierarchical_allocator_tests.cpp, line 2406
> > <https://reviews.apache.org/r/43880/diff/14/?file=1279201#file1279201line2406>
> >
> >     s/users/frameworks

Done.


> On March 4, 2016, 2:05 p.m., Alexander Rukletsov wrote:
> > src/tests/hierarchical_allocator_tests.cpp, lines 2420-2424
> > <https://reviews.apache.org/r/43880/diff/14/?file=1279201#file1279201line2420>
> >
> >     Can you move it up where you check allocated/* metrics? Adding a framework shouldn't
influence this counter.

I think this moved down here in some cleanup, I moved it back up.


> On March 4, 2016, 2:05 p.m., Alexander Rukletsov wrote:
> > src/tests/hierarchical_allocator_tests.cpp, lines 2449-2453
> > <https://reviews.apache.org/r/43880/diff/14/?file=1279201#file1279201line2449>
> >
> >     I think resources will be offered to `framework2`. But is it actually important?
I think you can kill this comment altogether (also the similar one above), and add a short
one around `settle()`.

Done.


> On March 4, 2016, 2:05 p.m., Alexander Rukletsov wrote:
> > src/tests/hierarchical_allocator_tests.cpp, line 2463
> > <https://reviews.apache.org/r/43880/diff/14/?file=1279201#file1279201line2463>
> >
> >     Feel free to kill this line.

It didn't leave a trace.


> On March 4, 2016, 2:05 p.m., Alexander Rukletsov wrote:
> > src/tests/hierarchical_allocator_tests.cpp, line 2491
> > <https://reviews.apache.org/r/43880/diff/14/?file=1279201#file1279201line2491>
> >
> >     /cluster/cluster state/

Stated as such.


- Benjamin


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


On March 4, 2016, 5:01 p.m., Benjamin Bannier wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43880/
> -----------------------------------------------------------
> 
> (Updated March 4, 2016, 5:01 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Ben Mahler.
> 
> 
> Bugs: MESOS-4720
>     https://issues.apache.org/jira/browse/MESOS-4720
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> See summary.
> 
> 
> Diffs
> -----
> 
>   docs/monitoring.md 827f7073204fcf8575ca980a5571c8be4f5e4110 
>   src/master/allocator/mesos/hierarchical.hpp 3043888630b066505410d3b32c5b3f813cc458c1

>   src/master/allocator/mesos/hierarchical.cpp 70291075c00a9a557529c2562dedcfc6c6c3ec32

>   src/master/allocator/mesos/metrics.hpp PRE-CREATION 
>   src/master/allocator/mesos/metrics.cpp PRE-CREATION 
>   src/tests/hierarchical_allocator_tests.cpp 3e4ad31925e1b815a74d67fa3962d23fa5bc89d1

> 
> Diff: https://reviews.apache.org/r/43880/diff/
> 
> 
> Testing
> -------
> 
> make check (OS X)
> 
> I confirmed that this does not lead to general performance regressions in the allocator;
this is partially expected since the added code only inserts metrics in the allocator while
the actual work is perform asynchronously. These tests where performed with `HierarchicalAllocator_BENCHMARK_Test.DeclineOffers`
on an optimized build under OS X using clang(trunk) as compiler.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>


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