[kudu-CR] KUDU-2279 (part 2): metrics: only emit changed metrics in metrics log
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 Reviewed-by: Mike Percy --- 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 Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Will Berkeley
[kudu-CR] KUDU-2279 (part 2): metrics: only emit changed metrics in metrics log
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 Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Tue, 06 Feb 2018 07:58:16 + Gerrit-HasComments: No
[kudu-CR] KUDU-2279 (part 2): metrics: only emit changed metrics in metrics log
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 Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Tue, 06 Feb 2018 07:36:59 + Gerrit-HasComments: No
[kudu-CR] KUDU-2279 (part 2): metrics: only emit changed metrics in metrics log
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
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy
Gerrit-Reviewer: Todd Lipcon
Gerrit-Reviewer: Will Berkeley
Gerrit-Comment-Date: Sat, 03 Feb 2018 07:02:06 +
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2279 (part 2): metrics: only emit changed metrics in metrics log
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 Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Will Berkeley
[kudu-CR] KUDU-2279 (part 2): metrics: only emit changed metrics in metrics log
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 Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Sat, 03 Feb 2018 01:49:40 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-2279 (part 2): metrics: only emit changed metrics in metrics log
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
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy
Gerrit-Reviewer: Todd Lipcon
Gerrit-Reviewer: Will Berkeley
Gerrit-Comment-Date: Sat, 03 Feb 2018 01:07:12 +
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2279 (part 2): metrics: only emit changed metrics in metrics log
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
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy
Gerrit-Reviewer: Will Berkeley
Gerrit-Comment-Date: Fri, 02 Feb 2018 20:21:19 +
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2279 (part 2): metrics: only emit changed metrics in metrics log
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 Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Fri, 02 Feb 2018 20:17:13 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-2279 (part 2): metrics: only emit changed metrics in metrics log
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 threadT1 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
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy
Gerrit-Reviewer: Will Berkeley
Gerrit-Comment-Date: Fri, 02 Feb 2018 20:16:10 +
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2279 (part 2): metrics: only emit changed metrics in metrics log
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 Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Will Berkeley
[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 Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Will Berkeley
