mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Ben Mahler <benjamin.mah...@gmail.com>
Subject Re: Review Request 44070: Allowed disabling metrics endpoint rate limiting via the environment.
Date Thu, 03 Mar 2016 05:30:57 GMT

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



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)
<https://reviews.apache.org/r/44070/#comment183586>

    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 =
        metrics::internal::MetricsProcess::instance();
    
      CHECK_NOTNULL(metricsProcess);
    ```
    
    Becomes:
    
    ```
      // Ensure metrics process is running.
      metrics::initialize();
    ```
    
    Two newlines below but one above?



3rdparty/libprocess/include/process/metrics/metrics.hpp (line 79)
<https://reviews.apache.org/r/44070/#comment183585>

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



3rdparty/libprocess/src/metrics/metrics.cpp (lines 19 - 30)
<https://reviews.apache.org/r/44070/#comment183612>

    Includes for Owned, numify, Duration?



3rdparty/libprocess/src/metrics/metrics.cpp (line 30)
<https://reviews.apache.org/r/44070/#comment183611>

    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)
<https://reviews.apache.org/r/44070/#comment183616>

    stale?



3rdparty/libprocess/src/metrics/metrics.cpp (line 41)
<https://reviews.apache.org/r/44070/#comment183587>

    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)
<https://reviews.apache.org/r/44070/#comment183588>

    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)
<https://reviews.apache.org/r/44070/#comment183592>

    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)
<https://reviews.apache.org/r/44070/#comment183593>

    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)
<https://reviews.apache.org/r/44070/#comment183613>

    We planned to add more endpoints in the future, so perhaps LIBPROCESS_METRICS_SNAPSHOT_ENDPOINT_RATE_LIMIT
would be more appropriate here. 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)
<https://reviews.apache.org/r/44070/#comment183595>

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



3rdparty/libprocess/src/metrics/metrics.cpp (lines 209 - 210)
<https://reviews.apache.org/r/44070/#comment183596>

    else if?



3rdparty/libprocess/src/metrics/metrics.cpp (line 210)
<https://reviews.apache.org/r/44070/#comment183597>

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



3rdparty/libprocess/src/metrics/metrics.cpp (lines 215 - 238)
<https://reviews.apache.org/r/44070/#comment183614>

    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)
<https://reviews.apache.org/r/44070/#comment183610>

    What is a slave? ;)



3rdparty/libprocess/src/metrics/metrics.cpp (line 253)
<https://reviews.apache.org/r/44070/#comment183615>

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


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