You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Benjamin Hindman <be...@berkeley.edu> on 2013/02/04 22:31:23 UTC

Re: Review Request: Resource Monitoring 2: Adding global statistics.

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



third_party/libprocess/include/process/statistics.hpp
<https://reviews.apache.org/r/9090/#comment34372>

    I'd prefer to namespace statistics. I had imagined just an instance called 'statistics' within the process namespace. Two wins I see here. First, people see usages like 'process::statistics.set(...)' and they see it's clearly provided by libprocess. Likewise, they might do a 'using process::statistics' to just bring in the 'statistics' object from the process namespace. I recognize the desire for a dynamically allocated instance of Statistics in order to avoid cleanup issues (are there any?). Hence, a macro provides a nice means to handle that level of indirection, but maybe it's not so bad if people have to do 'statistics->set()'. On the flip side, one could imagine adding a statistics namespace that mirrors the Statistics interface yielding usage like 'process::statistics::set()'. I probably like this best, but recognize the unfortunate need to replicate the interface (and keep it in sync, etc).



third_party/libprocess/src/statistics.cpp
<https://reviews.apache.org/r/9090/#comment34370>

    This is a departure from our standard style ... what caused you to add the CHECKs?


- Benjamin Hindman


On Jan. 28, 2013, 10:49 p.m., Ben Mahler wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/9090/
> -----------------------------------------------------------
> 
> (Updated Jan. 28, 2013, 10:49 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman and Vinod Kone.
> 
> 
> Description
> -------
> 
> This adds a global Statistics object for libprocess, which enables centralized stats across Processes.
> 
> I'd love to eventually plumb stats into libprocess and export to the webui! (MESOS-320)
> 
> 
> This addresses bug MESOS-324.
>     https://issues.apache.org/jira/browse/MESOS-324
> 
> 
> Diffs
> -----
> 
>   third_party/libprocess/include/process/statistics.hpp 9e3041a6e2a8ef022eacacad00bc4d974a8e33c9 
>   third_party/libprocess/src/process.cpp 72e437862ee0b35126c16d32bec79ef76a4e2b23 
>   third_party/libprocess/src/statistics.cpp 2fe8af83c6c63a0fa8cb2e9636f9289f0e3d7f2f 
> 
> Diff: https://reviews.apache.org/r/9090/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Ben Mahler
> 
>


Re: Review Request: Resource Monitoring 2: Adding global statistics.

Posted by Ben Mahler <be...@gmail.com>.

> On Feb. 4, 2013, 9:31 p.m., Benjamin Hindman wrote:
> > third_party/libprocess/src/statistics.cpp, lines 60-61
> > <https://reviews.apache.org/r/9090/diff/2/?file=252492#file252492line60>
> >
> >     This is a departure from our standard style ... what caused you to add the CHECKs?

I don't need these CHECKs anymore, as originally without the leading '/' these were not routing! (Returning false).

As a note, we may want to make this a Try or have all callers use CHECK. As long as callers are not ignoring the return value.


> On Feb. 4, 2013, 9:31 p.m., Benjamin Hindman wrote:
> > third_party/libprocess/include/process/statistics.hpp, line 14
> > <https://reviews.apache.org/r/9090/diff/2/?file=252490#file252490line14>
> >
> >     I'd prefer to namespace statistics. I had imagined just an instance called 'statistics' within the process namespace. Two wins I see here. First, people see usages like 'process::statistics.set(...)' and they see it's clearly provided by libprocess. Likewise, they might do a 'using process::statistics' to just bring in the 'statistics' object from the process namespace. I recognize the desire for a dynamically allocated instance of Statistics in order to avoid cleanup issues (are there any?). Hence, a macro provides a nice means to handle that level of indirection, but maybe it's not so bad if people have to do 'statistics->set()'. On the flip side, one could imagine adding a statistics namespace that mirrors the Statistics interface yielding usage like 'process::statistics::set()'. I probably like this best, but recognize the unfortunate need to replicate the interface (and keep it in sync, etc).

As discussed, added a process::statistics pointer to a heap allocated Statistics object.


- Ben


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


On Jan. 28, 2013, 10:49 p.m., Ben Mahler wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/9090/
> -----------------------------------------------------------
> 
> (Updated Jan. 28, 2013, 10:49 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman and Vinod Kone.
> 
> 
> Description
> -------
> 
> This adds a global Statistics object for libprocess, which enables centralized stats across Processes.
> 
> I'd love to eventually plumb stats into libprocess and export to the webui! (MESOS-320)
> 
> 
> This addresses bug MESOS-324.
>     https://issues.apache.org/jira/browse/MESOS-324
> 
> 
> Diffs
> -----
> 
>   third_party/libprocess/include/process/statistics.hpp 9e3041a6e2a8ef022eacacad00bc4d974a8e33c9 
>   third_party/libprocess/src/process.cpp 72e437862ee0b35126c16d32bec79ef76a4e2b23 
>   third_party/libprocess/src/statistics.cpp 2fe8af83c6c63a0fa8cb2e9636f9289f0e3d7f2f 
> 
> Diff: https://reviews.apache.org/r/9090/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Ben Mahler
> 
>