Todd Lipcon 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:

(7 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 w
the original reasoning for it is that we might start putting other samples into 
the log - eg a dump of entities, or periodic rpcz traces, etc. So that's why I 
did this somewhat strange format.

Agree it's strange but we already have some tools for parsing it so don't want 
to change it now.


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 `op
the reason for the 'prev_log_epoch' thing is that, in the odd case that we 
weren't able to write to the log, we want to keep using the same old epoch 
until we get a successful write.

Let me see if I can make the initial epoch 0 to clean this up a bit.


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 w
oops. yea, I'll see if I can clean up the test names a bit.


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@570
PS2, Line 570: base::subtle::NoBarrier_Load(&m_epoch_)
> Why use these atomics and not C++11 atomics? (Actual question, not review-d
will see if I can switch. I get nervous abotu the C++11 atomics because I don't 
understand how the C++11 memory model constants map to x86 assembly 
instructions, but maybe I need to get over my fear and spend some time with an 
assembler.


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);
             :     }
> Sorry, this interleaving isn't right.
yea, I guess what we actually need is a CAS loop for the 'store' portion to 
ensure that we never move it backwards in an odd series of races.


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();
> Also, if we want to make sure hitting /metrics doesn't change the epoch, we
hm, yea I think this can just be removed. I think it's a holdover from a 
previous implementation I had.

That said, I don't think this can cause it to miss any metrics -- the idea of 
the epoch is that it's free to advance whenever it wants, and the metrics 
logging thread won't miss changes, because it's always passing a lower bound, 
not an equality check, right?

Agreed that offering the ability to query the epoch and set these new options 
in the /metrics endpoint might be useful, will defer that work though for now.


http://gerrit.cloudera.org:8080/#/c/9176/2/src/kudu/util/metrics.cc@499
PS2, Line 499: 1
> Can we start at 0?
will see if I can do that.



--
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: Todd Lipcon <t...@apache.org>
Gerrit-Reviewer: Will Berkeley <wdberke...@gmail.com>
Gerrit-Comment-Date: Sat, 03 Feb 2018 01:07:12 +0000
Gerrit-HasComments: Yes

Reply via email to