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 3: Code-Review+1

(3 comments)

LGTM but I think Mike wanted to take a look.

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@630
PS2, Line 630: JsonWriter writer(&buf, JsonWriter::COMPACT);
             :     Status s = metric_registry_->WriteAsJson(&writer, {"*"}, 
opts);
             :     if (!s.ok()) {
> the reason for the 'prev_log_epoch' thing is that, in the odd case that we
Gotcha. I didn't pick up on that the first time.


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

http://gerrit.cloudera.org:8080/#/c/9176/3/src/kudu/util/metrics-test.cc@66
PS3, Line 66: test_counter, "My Test Counter"
Thanks for doing this rote stuff.


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: return Status::OK();
> hm, yea I think this can just be removed. I think it's a holdover from a pr
Yup, confused incrementing a metric's epoch with the metric subsystem epoch :)



--
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: 3
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 07:02:06 +0000
Gerrit-HasComments: Yes

Reply via email to