You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Todd Lipcon (Code Review)" <ge...@cloudera.org> on 2018/02/01 03:38:24 UTC
[kudu-CR] KUDU-2279 (part 2): metrics: only emit changed metrics in metrics log
Hello Will Berkeley, Mike Percy,
I'd like you to do a code review. Please visit
http://gerrit.cloudera.org:8080/9176
to review the following change.
Change subject: KUDU-2279 (part 2): metrics: only emit changed metrics in metrics log
......................................................................
KUDU-2279 (part 2): metrics: only emit changed metrics in metrics log
This adds a global 'metrics epoch' which can be externally incremented.
When metrics are modified, they remember the epoch of their most recent
modification.
When we dump metrics, we can pass a lower bound in order to see only
metrics which have been modified in or after a given epoch.
This patch updates the metrics logging to only emit metrics that have
changed in each successive line. This should substantially reduce the
size and CPU cost of metric logging on servers with thousands of
tablets.
Change-Id: Ia26be99a1fa96d52e2ca0905844d56c096d3778e
---
M src/kudu/server/server_base.cc
M src/kudu/util/metrics-test.cc
M src/kudu/util/metrics.cc
M src/kudu/util/metrics.h
4 files changed, 106 insertions(+), 7 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/76/9176/1
--
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: newchange
Gerrit-Change-Id: Ia26be99a1fa96d52e2ca0905844d56c096d3778e
Gerrit-Change-Number: 9176
Gerrit-PatchSet: 1
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
[kudu-CR] KUDU-2279 (part 2): metrics: only emit changed metrics in metrics log
Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
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 <to...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Sat, 03 Feb 2018 07:02:06 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2279 (part 2): metrics: only emit changed metrics in metrics log
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Hello Will Berkeley, Mike Percy, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/9176
to look at the new patch set (#3).
Change subject: KUDU-2279 (part 2): metrics: only emit changed metrics in metrics log
......................................................................
KUDU-2279 (part 2): metrics: only emit changed metrics in metrics log
This adds a global 'metrics epoch' which can be externally incremented.
When metrics are modified, they remember the epoch of their most recent
modification.
When we dump metrics, we can pass a lower bound in order to see only
metrics which have been modified in or after a given epoch.
This patch updates the metrics logging to only emit metrics that have
changed in each successive line. This should substantially reduce the
size and CPU cost of metric logging on servers with thousands of
tablets.
Change-Id: Ia26be99a1fa96d52e2ca0905844d56c096d3778e
---
M src/kudu/server/server_base.cc
M src/kudu/util/metrics-test.cc
M src/kudu/util/metrics.cc
M src/kudu/util/metrics.h
4 files changed, 141 insertions(+), 26 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/76/9176/3
--
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: newpatchset
Gerrit-Change-Id: Ia26be99a1fa96d52e2ca0905844d56c096d3778e
Gerrit-Change-Number: 9176
Gerrit-PatchSet: 3
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
[kudu-CR] KUDU-2279 (part 2): metrics: only emit changed metrics in metrics log
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
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 <to...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Sat, 03 Feb 2018 01:07:12 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2279 (part 2): metrics: only emit changed metrics in metrics log
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
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@630
PS2, Line 630: opts.only_modified_since_epoch = prev_log_epoch + 1;
: int64_t this_log_epoch = Metric::current_epoch();
: Metric::IncrementEpoch();
> the reason for the 'prev_log_epoch' thing is that, in the odd case that we
Done
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@332
PS2, Line 332: epoch_before_modify
> This is the epoch of/containing the modification, not the epoch before.
Done
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`.
Done
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.
Done
http://gerrit.cloudera.org:8080/#/c/9176/2/src/kudu/util/metrics.h@570
PS2, Line 570: base::subtle::NoBarrier_Load(&m_epoch_)
> will see if I can switch. I get nervous abotu the C++11 atomics because I d
Done
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();
> hm, yea I think this can just be removed. I think it's a holdover from a pr
Done
http://gerrit.cloudera.org:8080/#/c/9176/2/src/kudu/util/metrics.cc@499
PS2, Line 499: 1
> will see if I can do that.
Done
--
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 <to...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Sat, 03 Feb 2018 01:49:40 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2279 (part 2): metrics: only emit changed metrics in metrics log
Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
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:
(1 comment)
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 should add a quick test for it.
--
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 <to...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Fri, 02 Feb 2018 20:17:13 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2279 (part 2): metrics: only emit changed metrics in metrics log
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Hello Will Berkeley, Mike Percy, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/9176
to look at the new patch set (#2).
Change subject: KUDU-2279 (part 2): metrics: only emit changed metrics in metrics log
......................................................................
KUDU-2279 (part 2): metrics: only emit changed metrics in metrics log
This adds a global 'metrics epoch' which can be externally incremented.
When metrics are modified, they remember the epoch of their most recent
modification.
When we dump metrics, we can pass a lower bound in order to see only
metrics which have been modified in or after a given epoch.
This patch updates the metrics logging to only emit metrics that have
changed in each successive line. This should substantially reduce the
size and CPU cost of metric logging on servers with thousands of
tablets.
Change-Id: Ia26be99a1fa96d52e2ca0905844d56c096d3778e
---
M src/kudu/server/server_base.cc
M src/kudu/util/metrics-test.cc
M src/kudu/util/metrics.cc
M src/kudu/util/metrics.h
4 files changed, 109 insertions(+), 7 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/76/9176/2
--
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: newpatchset
Gerrit-Change-Id: Ia26be99a1fa96d52e2ca0905844d56c096d3778e
Gerrit-Change-Number: 9176
Gerrit-PatchSet: 2
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
[kudu-CR] KUDU-2279 (part 2): metrics: only emit changed metrics in metrics log
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/9176 )
Change subject: KUDU-2279 (part 2): metrics: only emit changed metrics in metrics log
......................................................................
KUDU-2279 (part 2): metrics: only emit changed metrics in metrics log
This adds a global 'metrics epoch' which can be externally incremented.
When metrics are modified, they remember the epoch of their most recent
modification.
When we dump metrics, we can pass a lower bound in order to see only
metrics which have been modified in or after a given epoch.
This patch updates the metrics logging to only emit metrics that have
changed in each successive line. This should substantially reduce the
size and CPU cost of metric logging on servers with thousands of
tablets.
Change-Id: Ia26be99a1fa96d52e2ca0905844d56c096d3778e
Reviewed-on: http://gerrit.cloudera.org:8080/9176
Tested-by: Kudu Jenkins
Reviewed-by: Will Berkeley <wd...@gmail.com>
Reviewed-by: Mike Percy <mp...@apache.org>
---
M src/kudu/server/server_base.cc
M src/kudu/util/metrics-test.cc
M src/kudu/util/metrics.cc
M src/kudu/util/metrics.h
4 files changed, 141 insertions(+), 26 deletions(-)
Approvals:
Kudu Jenkins: Verified
Will Berkeley: Looks good to me, but someone else must approve
Mike Percy: Looks good to me, approved
--
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: merged
Gerrit-Change-Id: Ia26be99a1fa96d52e2ca0905844d56c096d3778e
Gerrit-Change-Number: 9176
Gerrit-PatchSet: 4
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
[kudu-CR] KUDU-2279 (part 2): metrics: only emit changed metrics in metrics log
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
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 3:
Yea, though we do dump all of the entities even if none of their metrics changed so we'll see them disappear when retired.
--
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 <to...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Tue, 06 Feb 2018 07:58:16 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-2279 (part 2): metrics: only emit changed metrics in metrics log
Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
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:
(1 comment)
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@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:
Sorry, this interleaving isn't right.
First, suppose m_epoch_ starts at -1 (or increment everything by 1 and start it at 0).
Second, T1 has to do its test, before T2 runs at all, then do the set after T2 finishes.
--
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 <to...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Fri, 02 Feb 2018 20:21:19 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2279 (part 2): metrics: only emit changed metrics in metrics log
Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
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 <to...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Fri, 02 Feb 2018 20:16:10 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2279 (part 2): metrics: only emit changed metrics in metrics log
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy 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+2
Nice feature. One downside is that it's not possible to tell the difference between metrics that were retired (tombstoned tablets) and metrics that haven't changed, but in most cases we probably won't care.
--
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 <to...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Tue, 06 Feb 2018 07:36:59 +0000
Gerrit-HasComments: No