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 44260: Moved metrics of the hierarchical allocator to its own file.
Date Sat, 05 Mar 2016 10:34:53 GMT


> On March 2, 2016, 2:58 p.m., Alexander Rukletsov wrote:
> > src/master/allocator/mesos/metrics.hpp, line 37
> > <https://reviews.apache.org/r/44260/diff/1/?file=1276467#file1276467line37>
> >
> >     You don't really need to call `.self()` here, there exists an `defer` override
taking process instance.
> 
> Benjamin Bannier wrote:
>     At least my clang does not trigger that overload.
> 
> Alexander Rukletsov wrote:
>     https://github.com/apache/mesos/blob/9bbba94021dde42c9d9d1fa0662462c364797018/3rdparty/libprocess/include/process/defer.hpp#L177
> 
> Benjamin Bannier wrote:
>     The issue I see is below when directly using the `Process` here. Note that this works
for code in `HierarchicalAllocatorProcess`.
>     
>     || In file included from ../../src/master/allocator/mesos/metrics.cpp:17:
>     || In file included from ../../src/master/allocator/mesos/metrics.hpp:26:
>     || In file included from ../../3rdparty/libprocess/include/process/metrics/gauge.hpp:19:
>     || In file included from ../../3rdparty/libprocess/include/process/defer.hpp:19:
>     || ../../3rdparty/libprocess/include/process/deferred.hpp:110:14: error: no matching
conversion for functional-style cast from 'double (mesos::internal::master::allocator::internal::HierarchicalAllocatorProcess::*const)()'
to 'std::function<Future<double> ()>'
>     ||       return std::function<R()>(f);
>     ||              ^~~~~~~~~~~~~~~~~~~~
>     || ../../src/master/allocator/mesos/metrics.cpp:38:9: note: in instantiation of function
template specialization 'process::_Deferred<double (mesos::internal::master::allocator::internal::HierarchicalAllocatorProcess::*)()>::operator
Deferred<process::Future<double> >' requested here
>     ||         process::defer(
>     ||         ^
>     || /XYZ/src/llvm/P/bin/../include/c++/v1/functional:1593:5: note: candidate constructor
not viable: no known conversion from 'double (mesos::internal::master::allocator::internal::HierarchicalAllocatorProcess::*const)()'
to 'nullptr_t' for 1st argument
>     ||     function(nullptr_t) _NOEXCEPT : __f_(0) {}
>     ||     ^
>     || /XYZ/src/llvm/P/bin/../include/c++/v1/functional:1594:5: note: candidate constructor
not viable: no known conversion from 'double (mesos::internal::master::allocator::internal::HierarchicalAllocatorProcess::*const)()'
to 'const std::__1::function<process::Future<double> ()>' for 1st argument
>     ||     function(const function&);
>     ||     ^
>     || /XYZ/src/llvm/P/bin/../include/c++/v1/functional:1595:5: note: candidate constructor
not viable: no known conversion from 'double (mesos::internal::master::allocator::internal::HierarchicalAllocatorProcess::*const)()'
to 'std::__1::function<process::Future<double> ()>' for 1st argument
>     ||     function(function&&) _NOEXCEPT;
>     ||     ^
>     || /XYZ/src/llvm/P/bin/../include/c++/v1/functional:1599:41: note: candidate template
ignored: disabled by 'enable_if' [with _Fp = double (mesos::internal::master::allocator::internal::HierarchicalAllocatorProcess::*)()]
>     ||                                         __callable<_Fp>::value &&
>     ||                                         ^
>     || /XYZ/src/llvm/P/bin/../include/c++/v1/functional:1605:7: note: candidate constructor
template not viable: requires 2 arguments, but 1 was provided
>     ||       function(allocator_arg_t, const _Alloc&) _NOEXCEPT : __f_(0) {}
>     ||       ^
>     || /XYZ/src/llvm/P/bin/../include/c++/v1/functional:1608:7: note: candidate constructor
template not viable: requires 3 arguments, but 1 was provided
>     ||       function(allocator_arg_t, const _Alloc&, nullptr_t) _NOEXCEPT : __f_(0)
{}
>     ||       ^
>     || /XYZ/src/llvm/P/bin/../include/c++/v1/functional:1610:7: note: candidate constructor
template not viable: requires 3 arguments, but 1 was provided
>     ||       function(allocator_arg_t, const _Alloc&, const function&);
>     ||       ^
>     || /XYZ/src/llvm/P/bin/../include/c++/v1/functional:1612:7: note: candidate constructor
template not viable: requires 3 arguments, but 1 was provided
>     ||       function(allocator_arg_t, const _Alloc&, function&&);
>     ||       ^
>     || /XYZ/src/llvm/P/bin/../include/c++/v1/functional:1614:7: note: candidate constructor
template not viable: requires at least 3 arguments, but 1 was provided
>     ||       function(allocator_arg_t, const _Alloc& __a, _Fp __f,
>     ||       ^
>     || /XYZ/src/llvm/P/bin/../include/c++/v1/functional:1591:5: note: candidate constructor
not viable: requires 0 arguments, but 1 was provided
>     ||     function() _NOEXCEPT : __f_(0) {}
>     ||     ^
>     || In file included from ../../src/master/allocator/mesos/metrics.cpp:17:
>     || In file included from ../../src/master/allocator/mesos/metrics.hpp:26:
>     || In file included from ../../3rdparty/libprocess/include/process/metrics/gauge.hpp:19:
>     || In file included from ../../3rdparty/libprocess/include/process/defer.hpp:19:
>     || ../../3rdparty/libprocess/include/process/deferred.hpp:118:39: error: no matching
conversion for functional-style cast from 'double (mesos::internal::master::allocator::internal::HierarchicalAllocatorProcess::*const)()'
to 'std::function<Future<double> ()>'
>     ||           return dispatch(pid_.get(), std::function<R()>(f_));
>     ||                                       ^~~~~~~~~~~~~~~~~~~~~
>     || /XYZ/src/llvm/P/bin/../include/c++/v1/functional:1593:5: note: candidate constructor
not viable: no known conversion from 'double (mesos::internal::master::allocator::internal::HierarchicalAllocatorProcess::*const)()'
to 'nullptr_t' for 1st argument
>     ||     function(nullptr_t) _NOEXCEPT : __f_(0) {}
>     ||     ^
>     || /XYZ/src/llvm/P/bin/../include/c++/v1/functional:1594:5: note: candidate constructor
not viable: no known conversion from 'double (mesos::internal::master::allocator::internal::HierarchicalAllocatorProcess::*const)()'
to 'const std::__1::function<process::Future<double> ()>' for 1st argument
>     ||     function(const function&);
>     ||     ^
>     || /XYZ/src/llvm/P/bin/../include/c++/v1/functional:1595:5: note: candidate constructor
not viable: no known conversion from 'double (mesos::internal::master::allocator::internal::HierarchicalAllocatorProcess::*const)()'
to 'std::__1::function<process::Future<double> ()>' for 1st argument
>     ||     function(function&&) _NOEXCEPT;
>     ||     ^
>     || /XYZ/src/llvm/P/bin/../include/c++/v1/functional:1599:41: note: candidate template
ignored: disabled by 'enable_if' [with _Fp = double (mesos::internal::master::allocator::internal::HierarchicalAllocatorProcess::*)()]
>     ||                                         __callable<_Fp>::value &&
>     ||                                         ^
>     || /XYZ/src/llvm/P/bin/../include/c++/v1/functional:1605:7: note: candidate constructor
template not viable: requires 2 arguments, but 1 was provided
>     ||       function(allocator_arg_t, const _Alloc&) _NOEXCEPT : __f_(0) {}
>     ||       ^
>     || /XYZ/src/llvm/P/bin/../include/c++/v1/functional:1608:7: note: candidate constructor
template not viable: requires 3 arguments, but 1 was provided
>     ||       function(allocator_arg_t, const _Alloc&, nullptr_t) _NOEXCEPT : __f_(0)
{}
>     ||       ^
>     || /XYZ/src/llvm/P/bin/../include/c++/v1/functional:1610:7: note: candidate constructor
template not viable: requires 3 arguments, but 1 was provided
>     ||       function(allocator_arg_t, const _Alloc&, const function&);
>     ||       ^
>     || /XYZ/src/llvm/P/bin/../include/c++/v1/functional:1612:7: note: candidate constructor
template not viable: requires 3 arguments, but 1 was provided
>     ||       function(allocator_arg_t, const _Alloc&, function&&);
>     ||       ^
>     || /XYZ/src/llvm/P/bin/../include/c++/v1/functional:1614:7: note: candidate constructor
template not viable: requires at least 3 arguments, but 1 was provided
>     ||       function(allocator_arg_t, const _Alloc& __a, _Fp __f,
>     ||       ^
>     || /XYZ/src/llvm/P/bin/../include/c++/v1/functional:1591:5: note: candidate constructor
not viable: requires 0 arguments, but 1 was provided
>     ||     function() _NOEXCEPT : __f_(0) {}
>     ||     ^
>     || 1 warning and 2 errors generated.
>     || Error while processing /XYZ/src/mesos/src/master/allocator/mesos/metrics.cpp.
>     || warning: 0.28.0": 'linker' input unused
>     || In file included from <built-in>:356:
>     || <command line>:4:24: warning: missing terminating '"' character [-Winvalid-pp-token]
>     || #define PACKAGE_STRING "mesos
>     ||                        ^
>     || In file included from ../../src/master/allocator/mesos/metrics.cpp:17:
>     || In file included from ../../src/master/allocator/mesos/metrics.hpp:26:
>     || In file included from ../../3rdparty/libprocess/include/process/metrics/gauge.hpp:19:
>     || In file included from ../../3rdparty/libprocess/include/process/defer.hpp:19:
>     || ../../3rdparty/libprocess/include/process/deferred.hpp:110:14: error: no matching
conversion for functional-style cast from 'double (mesos::internal::master::allocator::internal::HierarchicalAllocatorProcess::*const)()'
to 'std::function<Future<double> ()>'
>     ||       return std::function<R()>(f);
>     ||              ^~~~~~~~~~~~~~~~~~~~
>     || ../../src/master/allocator/mesos/metrics.cpp:38:9: note: in instantiation of function
template specialization 'process::_Deferred<double (mesos::internal::master::allocator::internal::HierarchicalAllocatorProcess::*)()>::operator
Deferred<process::Future<double> >' requested here
>     ||         process::defer(
>     ||         ^
>     || /XYZ/src/llvm/P/bin/../include/c++/v1/functional:1593:5: note: candidate constructor
not viable: no known conversion from 'double (mesos::internal::master::allocator::internal::HierarchicalAllocatorProcess::*const)()'
to 'nullptr_t' for 1st argument
>     ||     function(nullptr_t) _NOEXCEPT : __f_(0) {}
>     ||     ^
>     || /XYZ/src/llvm/P/bin/../include/c++/v1/functional:1594:5: note: candidate constructor
not viable: no known conversion from 'double (mesos::internal::master::allocator::internal::HierarchicalAllocatorProcess::*const)()'
to 'const std::__1::function<process::Future<double> ()>' for 1st argument
>     ||     function(const function&);
>     ||     ^
>     || /XYZ/src/llvm/P/bin/../include/c++/v1/functional:1595:5: note: candidate constructor
not viable: no known conversion from 'double (mesos::internal::master::allocator::internal::HierarchicalAllocatorProcess::*const)()'
to 'std::__1::function<process::Future<double> ()>' for 1st argument
>     ||     function(function&&) _NOEXCEPT;
>     ||     ^
>     || /XYZ/src/llvm/P/bin/../include/c++/v1/functional:1599:41: note: candidate template
ignored: disabled by 'enable_if' [with _Fp = double (mesos::internal::master::allocator::internal::HierarchicalAllocatorProcess::*)()]
>     ||                                         __callable<_Fp>::value &&
>     ||                                         ^
>     || /XYZ/src/llvm/P/bin/../include/c++/v1/functional:1605:7: note: candidate constructor
template not viable: requires 2 arguments, but 1 was provided
>     ||       function(allocator_arg_t, const _Alloc&) _NOEXCEPT : __f_(0) {}
>     ||       ^
>     || /XYZ/src/llvm/P/bin/../include/c++/v1/functional:1608:7: note: candidate constructor
template not viable: requires 3 arguments, but 1 was provided
>     ||       function(allocator_arg_t, const _Alloc&, nullptr_t) _NOEXCEPT : __f_(0)
{}
>     ||       ^
>     || /XYZ/src/llvm/P/bin/../include/c++/v1/functional:1610:7: note: candidate constructor
template not viable: requires 3 arguments, but 1 was provided
>     ||       function(allocator_arg_t, const _Alloc&, const function&);
>     ||       ^
>     || /XYZ/src/llvm/P/bin/../include/c++/v1/functional:1612:7: note: candidate constructor
template not viable: requires 3 arguments, but 1 was provided
>     ||       function(allocator_arg_t, const _Alloc&, function&&);
>     ||       ^
>     || /XYZ/src/llvm/P/bin/../include/c++/v1/functional:1614:7: note: candidate constructor
template not viable: requires at least 3 arguments, but 1 was provided
>     ||       function(allocator_arg_t, const _Alloc& __a, _Fp __f,
>     ||       ^
>     || /XYZ/src/llvm/P/bin/../include/c++/v1/functional:1591:5: note: candidate constructor
not viable: requires 0 arguments, but 1 was provided
>     ||     function() _NOEXCEPT : __f_(0) {}
>     ||     ^
>     || In file included from ../../src/master/allocator/mesos/metrics.cpp:17:
>     || In file included from ../../src/master/allocator/mesos/metrics.hpp:26:
>     || In file included from ../../3rdparty/libprocess/include/process/metrics/gauge.hpp:19:
>     || In file included from ../../3rdparty/libprocess/include/process/defer.hpp:19:
>     || ../../3rdparty/libprocess/include/process/deferred.hpp:118:39: error: no matching
conversion for functional-style cast from 'double (mesos::internal::master::allocator::internal::HierarchicalAllocatorProcess::*const)()'
to 'std::function<Future<double> ()>'
>     ||           return dispatch(pid_.get(), std::function<R()>(f_));
>     ||                                       ^~~~~~~~~~~~~~~~~~~~~
>     || /XYZ/src/llvm/P/bin/../include/c++/v1/functional:1593:5: note: candidate constructor
not viable: no known conversion from 'double (mesos::internal::master::allocator::internal::HierarchicalAllocatorProcess::*const)()'
to 'nullptr_t' for 1st argument
>     ||     function(nullptr_t) _NOEXCEPT : __f_(0) {}
>     ||     ^
>     || /XYZ/src/llvm/P/bin/../include/c++/v1/functional:1594:5: note: candidate constructor
not viable: no known conversion from 'double (mesos::internal::master::allocator::internal::HierarchicalAllocatorProcess::*const)()'
to 'const std::__1::function<process::Future<double> ()>' for 1st argument
>     ||     function(const function&);
>     ||     ^
>     || /XYZ/src/llvm/P/bin/../include/c++/v1/functional:1595:5: note: candidate constructor
not viable: no known conversion from 'double (mesos::internal::master::allocator::internal::HierarchicalAllocatorProcess::*const)()'
to 'std::__1::function<process::Future<double> ()>' for 1st argument
>     ||     function(function&&) _NOEXCEPT;
>     ||     ^
>     || /XYZ/src/llvm/P/bin/../include/c++/v1/functional:1599:41: note: candidate template
ignored: disabled by 'enable_if' [with _Fp = double (mesos::internal::master::allocator::internal::HierarchicalAllocatorProcess::*)()]
>     ||                                         __callable<_Fp>::value &&
>     ||                                         ^
>     || /XYZ/src/llvm/P/bin/../include/c++/v1/functional:1605:7: note: candidate constructor
template not viable: requires 2 arguments, but 1 was provided
>     ||       function(allocator_arg_t, const _Alloc&) _NOEXCEPT : __f_(0) {}
>     ||       ^
>     || /XYZ/src/llvm/P/bin/../include/c++/v1/functional:1608:7: note: candidate constructor
template not viable: requires 3 arguments, but 1 was provided
>     ||       function(allocator_arg_t, const _Alloc&, nullptr_t) _NOEXCEPT : __f_(0)
{}
>     ||       ^
>     || /XYZ/src/llvm/P/bin/../include/c++/v1/functional:1610:7: note: candidate constructor
template not viable: requires 3 arguments, but 1 was provided
>     ||       function(allocator_arg_t, const _Alloc&, const function&);
>     ||       ^
>     || /XYZ/src/llvm/P/bin/../include/c++/v1/functional:1612:7: note: candidate constructor
template not viable: requires 3 arguments, but 1 was provided
>     ||       function(allocator_arg_t, const _Alloc&, function&&);
>     ||       ^
>     || /XYZ/src/llvm/P/bin/../include/c++/v1/functional:1614:7: note: candidate constructor
template not viable: requires at least 3 arguments, but 1 was provided
>     ||       function(allocator_arg_t, const _Alloc& __a, _Fp __f,
>     ||       ^
>     || /XYZ/src/llvm/P/bin/../include/c++/v1/functional:1591:5: note: candidate constructor
not viable: requires 0 arguments, but 1 was provided
>     ||     function() _NOEXCEPT : __f_(0) {}
>     ||     ^
>     || 1 warning and 2 errors generated.
>     || Error while processing /XYZ/src/mesos/src/master/allocator/mesos/metrics.cpp.
> 
> Benjamin Bannier wrote:
>     Somehow this doesn't seem to work with `HierarchicalAllocatorProcess` the way it
does with e.g., `Master`. Leaving in the self for now, but definitely worth investigating.
>     
>     Here's a reproducer:
>     
>     class P : public mesos::internal::master::allocator::MesosAllocatorProcess
>     {
>     public:
>       double hi() const { return 0; }
>       double ho() { return 0; }
>     };
>     
>     class M : public ProtobufProcess<M>
>     {
>     public:
>       double hi() { return 0; }
>     };
>     
>     void f(const P& p)
>     {
>       process::metrics::Gauge("defer via UPID and free function",
>                               process::defer(p, lambda::bind(&P::hi, &p)));
>     
>       // THIS DOES NOT WORK
>       // process::metrics::Gauge("defer via Process and member fct ptr",
>       //                         process::defer(p, P::hi)));
>     }
>     
>     void g(const M& m) {
>       // WORKS AS EXPECTED
>       process::metrics::Gauge("defer via Process and member fct ptr",
>                               process::defer(m, &M::hi));
>     }

A couple of things are going wrong here. Let's compare what's happening in the master metrics
(https://github.com/apache/mesos/blob/master/src/master/metrics.cpp#L43) and allocator metrics.

The inheritance hierarchies of `Master` and `HierarchicalAllocatorProcess` are

    Master -> ProtobufProcess<Master> -> Process<M> -> ProcessBase
    HierarchicalAllocatorProcess -> MesosAllocatorProcess -> Process<MesosAllocatorProcess>
-> ProcessBase

We also have `ProcessBase -> EventVisitor` but this is not relevant here.

The different `defer` overloads against which we could potentially match are (for some `typenames
T, R`)

     (1) Deferred<void()> defer(const Process<T>& process, void (T::*method)())
     (2) Deferred<void()> defer(const Process<T>* process, void (T::*method)())
     (3) Deferred<Future<R>()> defer(const PID<T>& pid, Future<R>
(T::*method)())
     (4) Deferred<Future<R>()> defer(const Process<T>& process, Future<R>
(T::*method)())
     (5) Deferred<void()> defer(const PID<T>& pid, void (T::*method)())
     (6) Deferred<Future<R>()> defer(const PID<T>& pid, R (T::*method)())
     (7) Deferred<Future<R>()> defer(const Process<T>& process, R (T::*method)())
     (8) Deferred<Future<R>()> defer(const Process<T>* process, R (T::*method)())
     (9) _Deferred<Future<R>()> defer(const UPID& pid, F&& f)
    (10) Deferred<Future<R>()> defer(const Process<T>* process, Future<R>
(T::*method)())

For the metrics use case here we are interrested only in methods returning a value, so overloads
(1), (2) and (5) are not interesting for us.


`defer` resolution with `Master`
--------------------------------

When we `defer` a call to say `Master::_uptime_secs` (returning a `double`)

   const Master& master;
   defer(master, &M::_uptime_secs);

we could match against overloads (4), (7), or (9). We can match against (4) or (7) since when
resolving we see e.g.,

    defer(const Process<Master>&, double (Master::*method);

where even though a derived to base cast is performed we still retain `Master` and can verify
that `_uptime_secs` is a suitable `Master` member function.
We can match against (9) since a `UPID` can implicitly be constructed from a `ProcessBase`
which also is just a derived to base cast away from `Master`.

Ultimatively (7) is choosen since it requires less conversions.


`defer` resolution with `HierarchicalAllocatorProcess`
------------------------------------------------------

When we `defer` a call to e.g., `HierarchicalAllocatorProcess::_event_queue_dispatches` derived
to base casts leave us with a `Process<MesosAllocatorProcess>` which does not have the
member function we are interested in. This excludes (4) and (7) from the matching set, so
the only remaining match is (9). When expanding this match further we get the failure we are
seeing due to peculiarities of `_Deferred` as opposed to `Deferred` since a `_Deferred` can
only be used to call free functions, and not member functions.

Using `self()` (which is overridden for `HierarchicalAllocatorProcess`) made this work since
we effectively perform a cast to `PID<HierarchicalAllocatorProcess>` and can match against
overloads returning `Deferred` instead of `_Deferred`.


Method `const`ness in overload resolution
-----------------------------------------

Independently, the matter here is made even worse by the fact that I later declared the new
getters in `HierarchicalAllocatorProcess` (e.g., `HierarchicalAllocatorProcess::_offer_filters`)
as `const`. Since there is not `defer` overload for `const` member functions, none of the
overloads taking `Process<T>` or `PID<T>` can match, and only the overload taking
a `UPID` and a free function can match. This did work here since I used lambdas to wrap the
member function calls (which is just the creation of free functions).


Summary
-------

In summary, when working with member function calls, the provided overloads of `defer` are
pretty restrictive:

* the overloads taking `Process<T>` and `R (T::*method)` require matching types in the
first and second argument for the match to be included. It should in principle be possible
to declare a more permissive overload taking a `Process<U>` and a `R (V::*method)` and
only later decide whether `U` and `V` are compatible.
* `defer`ing `const` member calls is overly cumbersome. A user should not feel tempted to
declare effectively `const` methods mutable, only to satisfy an overload in a low-level library
(here: `defer`).


- Benjamin


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


On March 3, 2016, 5:17 p.m., Benjamin Bannier wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44260/
> -----------------------------------------------------------
> 
> (Updated March 3, 2016, 5:17 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Ben Mahler.
> 
> 
> Bugs: MESOS-4718
>     https://issues.apache.org/jira/browse/MESOS-4718
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Moved metrics of the hierarchical allocator to its own file.
> 
> 
> Diffs
> -----
> 
>   src/CMakeLists.txt 0eabfad66f43adff12d08dbf34ca3db1b801c49e 
>   src/Makefile.am b30cc25f61856d6417437547baaa0bb338a30d63 
>   src/master/allocator/mesos/hierarchical.hpp 3043888630b066505410d3b32c5b3f813cc458c1

>   src/master/allocator/mesos/metrics.hpp PRE-CREATION 
>   src/master/allocator/mesos/metrics.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/44260/diff/
> 
> 
> Testing
> -------
> 
> `make distcheck` on OS X.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>


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