Will Berkeley has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9176 )

Change subject: KUDU-2279 (part 2): metrics: only emit changed metrics in 
metrics log
......................................................................


Patch Set 2:

(10 comments)

http://gerrit.cloudera.org:8080/#/c/9176/2/src/kudu/server/server_base.cc
File src/kudu/server/server_base.cc:

http://gerrit.cloudera.org:8080/#/c/9176/2/src/kudu/server/server_base.cc@623
PS2, Line 623: buf << "metrics " << GetCurrentTimeMicros() << " ";
The "metrics" string here is redundant for the metrics log. Plus, I think we 
should have each line of the metrics log be valid json, so there's one less 
step for a user to process it line-by-line. Can the time go into the metrics 
json?


http://gerrit.cloudera.org:8080/#/c/9176/2/src/kudu/server/server_base.cc@630
PS2, Line 630: opts.only_modified_since_epoch = prev_log_epoch + 1;
             :     int64_t this_log_epoch = Metric::current_epoch();
             :     Metric::IncrementEpoch();
If `Metric::current_epoch()` is initially 1, we actually jump epochs in 
`opts.only_modified_since_epoch` from 0 to 2. Besides, I think this code could 
be simpler and just set opts.only_modified_since_epoch to 
`Metric::current_epoch()` just before incrementing the current epoch.


http://gerrit.cloudera.org:8080/#/c/9176/2/src/kudu/util/metrics-test.cc
File src/kudu/util/metrics-test.cc:

http://gerrit.cloudera.org:8080/#/c/9176/2/src/kudu/util/metrics-test.cc@330
PS2, Line 330: bytes_seen = METRIC_reqs_pending
Mismatched names? Also, I'm weakly in favor of having canned metric names 
whenever the metric is fake or its identity is unimportant, like when testing 
metric subsystem internals. E.g. "test_counter" for counters.


http://gerrit.cloudera.org:8080/#/c/9176/2/src/kudu/util/metrics-test.cc@332
PS2, Line 332: epoch_before_modify
This is the epoch of/containing the modification, not the epoch before.


http://gerrit.cloudera.org:8080/#/c/9176/2/src/kudu/util/metrics-test.cc@347
PS2, Line 347: epoch_before_modify
This should be `new_epoch`.


http://gerrit.cloudera.org:8080/#/c/9176/2/src/kudu/util/metrics.h
File src/kudu/util/metrics.h:

http://gerrit.cloudera.org:8080/#/c/9176/2/src/kudu/util/metrics.h@567
PS2, Line 567: it's
Remove.


http://gerrit.cloudera.org:8080/#/c/9176/2/src/kudu/util/metrics.h@570
PS2, Line 570: base::subtle::NoBarrier_Load(&m_epoch_)
Why use these atomics and not C++11 atomics? (Actual question, not 
review-demand-phrased-as-question :))


http://gerrit.cloudera.org:8080/#/c/9176/2/src/kudu/util/metrics.h@595
PS2, Line 595: auto cur = current_epoch();
             :     if (base::subtle::NoBarrier_Load(&m_epoch_) < cur) {
             :       base::subtle::NoBarrier_Store(&m_epoch_, cur);
             :     }
I think the following interleaving could happen here:

current epoch = 0 initially
Metrics log thread    T1                  T2
                      incr metric
                      cur = 0
log for >= epoch 0
epoch++
                                            incr metric
                                            cur = 1
                                            TAS m_epoch_ = 1
                      TAS m_epoch_ = 0
log for >= epoch 1
epoch++

so now we might fail to log the metric's new values in epoch 1, and only see 
values from epoch >= 2 if the metric changes again.

Are you accepting this risk to keep the metric lock-free? AFAIK to fix it 
requires a bona fide lock and not just atomic ops.


http://gerrit.cloudera.org:8080/#/c/9176/2/src/kudu/util/metrics.cc
File src/kudu/util/metrics.cc:

http://gerrit.cloudera.org:8080/#/c/9176/2/src/kudu/util/metrics.cc@357
PS2, Line 357: Metric::IncrementEpoch();
With this here we increment the epoch every time someone hits /metrics and we 
double increment the current epoch when we log metrics, both of which can cause 
the metrics logging to miss metrics history. I think we just want to increment 
the epoch in the metrics logging thread.

Also, for another patch, might be nice to expose an epoch parameter in 
/metrics, to enable incremental gathering by a collector that can remember its 
epoch.


http://gerrit.cloudera.org:8080/#/c/9176/2/src/kudu/util/metrics.cc@499
PS2, Line 499: 1
Can we start at 0?



--
To view, visit http://gerrit.cloudera.org:8080/9176
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia26be99a1fa96d52e2ca0905844d56c096d3778e
Gerrit-Change-Number: 9176
Gerrit-PatchSet: 2
Gerrit-Owner: Todd Lipcon <t...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mpe...@apache.org>
Gerrit-Reviewer: Will Berkeley <wdberke...@gmail.com>
Gerrit-Comment-Date: Fri, 02 Feb 2018 20:16:10 +0000
Gerrit-HasComments: Yes

Reply via email to