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