This is an automatically generated e-mail. To reply, visit:

Thanks for the patience, I left some comments for some issues that remained. 
I'll get this committed shortly with adjustments based on the comments.

3rdparty/libprocess/include/process/metrics/metrics.hpp (lines 33 - 36)

    This should be called from process::initialize, if we'd like this to be 
consistent with Clock::initialize, mime::initialize, EventLoop::initialize:
      // TODO(bmahler): Consider initializing this consistently with
      // the other global Processes.
      metrics::internal::MetricsProcess* metricsProcess =
      // Ensure metrics process is running.
    Two newlines below but one above?

3rdparty/libprocess/include/process/metrics/metrics.hpp (line 79)

    Hm.. it's hard to tell what "setting up global infrastructure" mean? How 
    // Needed to access the private constructor.

3rdparty/libprocess/src/metrics/metrics.cpp (lines 19 - 30)

    Includes for Owned, numify, Duration?

3rdparty/libprocess/src/metrics/metrics.cpp (line 30)

    Pulling in the posix header means it won't compile on windows, so this 
should be the agnostic wrapper: 
    #include <stout/os.hpp>

3rdparty/libprocess/src/metrics/metrics.cpp (line 33)


3rdparty/libprocess/src/metrics/metrics.cpp (line 41)

    Looking around at the globals in the code base, I can't seem to find an 
instance of us naming the global "singleton". How about "metrics_process" to be 
consistent with the naming in proces.cpp for now?

3rdparty/libprocess/src/metrics/metrics.cpp (line 196)

    Since it's at the top of the header, can you move this to the top of the 
.cpp file?
    Also, whoops, brace should be on the next line here.

3rdparty/libprocess/src/metrics/metrics.cpp (lines 197 - 260)

    Hm.. any reason you introduced two ways to check that we only initialize 
once? The first check seems unreliable as Alexander mentioned, so is it a 
performance optimization?
    Also, it seems like the rate limiting parsing and creation should only 
occur when the once determines this is the only execution, right? Was this 
intentionally moved outside the once for some reason? Because as it stands we 
may create some unnecessary rate limiters and throw them away, which seems odd.

3rdparty/libprocess/src/metrics/metrics.cpp (line 201)

    How about s/rateLimiter/limiter/ here and elsewhere to be consistent with 
how it is named in the MetricsProcess?

3rdparty/libprocess/src/metrics/metrics.cpp (line 205)

    We planned to add more endpoints in the future, so perhaps 
For example, if we exposed an endoint for getting historical data, that may not 
need the same rate limiting since it doesn't hit components through gauges.

3rdparty/libprocess/src/metrics/metrics.cpp (lines 207 - 209)

    A comment that we default to 2 per second would be great here, noting that 
we default to this for backwards compatibility. The default was originally in 
place to apply a reasonable limit, and now we have to keep it for backwards 

3rdparty/libprocess/src/metrics/metrics.cpp (lines 209 - 210)

    else if?

3rdparty/libprocess/src/metrics/metrics.cpp (line 210)

    You can use -> instead of .get()., can you do a sweep within your code?
    // vs

3rdparty/libprocess/src/metrics/metrics.cpp (lines 215 - 238)

    It would be nice to do error messaging once rather than 3 places.
    I realize this was copied, but we tend to EXIT upon invalid user input. 
Since LOG(FATAL) prints a stack trace, users tend to think mesos has crashed 
because there is a bug in mesos.

3rdparty/libprocess/src/metrics/metrics.cpp (line 236)

    What is a slave? ;)

3rdparty/libprocess/src/metrics/metrics.cpp (line 253)

    Why did you change this from a pointer to a stack object? We don't allow 
static non-PODs.

- Ben Mahler

On March 2, 2016, 5:48 p.m., Benjamin Bannier wrote:
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44070/
> -----------------------------------------------------------
> (Updated March 2, 2016, 5:48 p.m.)
> Review request for mesos, Alexander Rukletsov and Ben Mahler.
> Bugs: MESOS-4776
>     https://issues.apache.org/jira/browse/MESOS-4776
> Repository: mesos
> Description
> -------
> Allowed disabling metrics endpoint rate limiting via the environment.
> Diffs
> -----
>   3rdparty/libprocess/include/process/metrics/metrics.hpp 
> 09b716be56eac38f75d79d917799c001aa0b205c 
>   3rdparty/libprocess/src/metrics/metrics.cpp 
> a9840083722dd6b7b6aab692ed449407ab125ac7 
> Diff: https://reviews.apache.org/r/44070/diff/
> Testing
> -------
> make check (OS X, not optimized)
> Thanks,
> Benjamin Bannier

Reply via email to