mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Benjamin Mahler <bmah...@apache.org>
Subject Re: Review Request 70783: Added debug logging for metrics which are slow to become ready.
Date Wed, 05 Jun 2019 01:05:59 GMT

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



Looks good, but I'm also wondering if there's a cleaner way to implement (see comment below).

Can you also include the impact this has on the benchmark (optimized build)?


3rdparty/libprocess/src/metrics/metrics.cpp
Lines 199-207 (patched)
<https://reviews.apache.org/r/70783/#comment302505>

    If we consolidate the logging between user timeout and slow timeout can we have one path?
    
    ```
      Future<Nothing> waited = <all futures ready OR user timeout OR slow timeout>;
    
      return waited
        .then(defer(self(), &Self::_snapshotHandler, ...));
    }
      
    _snapshotHandler(...)
    {
      // LOG(INFO) not ready futures after timeout (user or built-in SLOW one)
      ...
        
      Future<Nothing> waited = <all futures ready OR user timeout>;
    
      return waited
        .then(defer(self(), &Self::__snapshotHandler, ...));
    }
      
    __snapshotHandler(...)
    {
      ...
      return response;
    }
    ```
    
    We would log those not ready after either the user provided one, or the built-in slow
one, whichever is smaller (I guess we can use a min() for that too if we want). The downside
here seems to be that if a user specifies a small one and still lots of pull gauges are used,
will lead to a lot of INFO logging. Not sure how much we should worry about this case.
    
    We should probably name the endpoint handler functions and snapshot map function differently
or house the handler ones in an 'Http' sub-struct, a bit confusing right now.



3rdparty/libprocess/src/metrics/metrics.cpp
Lines 241-242 (original), 261-262 (patched)
<https://reviews.apache.org/r/70783/#comment302502>

    It's odd to have this similar logging be inconsistent (i.e. VLOG(1) intead of INFO and
1 per line instead of all in one line).



3rdparty/libprocess/src/metrics/metrics.cpp
Lines 303-304 (patched)
<https://reviews.apache.org/r/70783/#comment302506>

    Do we need these variables? Seems better without them


- Benjamin Mahler


On June 4, 2019, 2:54 p.m., Greg Mann wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70783/
> -----------------------------------------------------------
> 
> (Updated June 4, 2019, 2:54 p.m.)
> 
> 
> Review request for mesos, Benno Evers, Benjamin Mahler, Joseph Wu, and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> This patch adds logging which, after a hard-coded delay, prints
> the keys of any metrics which have not yet become ready
> following a request for the metrics snapshot.
> 
> 
> Diffs
> -----
> 
>   3rdparty/libprocess/include/process/metrics/metrics.hpp 75711edbaf46797e5eb54ba720ea11cf3de81522

>   3rdparty/libprocess/src/metrics/metrics.cpp 623d44adbe838f995ddbe89ee26f5bcc9c600be5

> 
> 
> Diff: https://reviews.apache.org/r/70783/diff/1/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Greg Mann
> 
>


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