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