[Impala-ASF-CR] IMPALA-7239: Disable smaps parsing by default
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/10884 ) Change subject: IMPALA-7239: Disable smaps parsing by default .. Patch Set 4: Verified+1 -- To view, visit http://gerrit.cloudera.org:8080/10884 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I235eddde8fe925866e0581b235752354a3f36d5b Gerrit-Change-Number: 10884 Gerrit-PatchSet: 4 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Wed, 11 Jul 2018 22:44:27 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7239: Disable smaps parsing by default
Impala Public Jenkins has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/10884 ) Change subject: IMPALA-7239: Disable smaps parsing by default .. IMPALA-7239: Disable smaps parsing by default Accessing smaps has proven to be too expensive to enable on all operating systems. Let's move the metrics behind a hidden flag so that they can be enabled for development if needed but won't affect production workloads. Change-Id: I235eddde8fe925866e0581b235752354a3f36d5b Reviewed-on: http://gerrit.cloudera.org:8080/10884 Reviewed-by: Impala Public Jenkins Tested-by: Impala Public Jenkins --- M be/src/common/init.cc M be/src/util/memory-metrics.cc M be/src/util/process-state-info.cc M be/src/util/process-state-info.h 4 files changed, 66 insertions(+), 46 deletions(-) Approvals: Impala Public Jenkins: Looks good to me, approved; Verified -- To view, visit http://gerrit.cloudera.org:8080/10884 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: I235eddde8fe925866e0581b235752354a3f36d5b Gerrit-Change-Number: 10884 Gerrit-PatchSet: 5 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Todd Lipcon
[Impala-ASF-CR] IMPALA-7239: Disable smaps parsing by default
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/10884 ) Change subject: IMPALA-7239: Disable smaps parsing by default .. Patch Set 4: Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/2796/ DRY_RUN=false -- To view, visit http://gerrit.cloudera.org:8080/10884 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I235eddde8fe925866e0581b235752354a3f36d5b Gerrit-Change-Number: 10884 Gerrit-PatchSet: 4 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Wed, 11 Jul 2018 19:23:21 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7239: Disable smaps parsing by default
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/10884 ) Change subject: IMPALA-7239: Disable smaps parsing by default .. Patch Set 4: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/10884 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I235eddde8fe925866e0581b235752354a3f36d5b Gerrit-Change-Number: 10884 Gerrit-PatchSet: 4 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Wed, 11 Jul 2018 19:23:20 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7239: Disable smaps parsing by default
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/10884 ) Change subject: IMPALA-7239: Disable smaps parsing by default .. Patch Set 3: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/10884 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I235eddde8fe925866e0581b235752354a3f36d5b Gerrit-Change-Number: 10884 Gerrit-PatchSet: 3 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Wed, 11 Jul 2018 18:53:05 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7239: Disable smaps parsing by default
Tim Armstrong has posted comments on this change. (
http://gerrit.cloudera.org:8080/10884 )
Change subject: IMPALA-7239: Disable smaps parsing by default
..
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/10884/2/be/src/util/process-state-info.cc
File be/src/util/process-state-info.cc:
http://gerrit.cloudera.org:8080/#/c/10884/2/be/src/util/process-state-info.cc@178
PS2, Line 178: string ProcessStateInfo::DebugString() const {
> I dunno where this is used, but would it make sense to not include the bits
It's used on the debug page. Fixed it.
--
To view, visit http://gerrit.cloudera.org:8080/10884
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I235eddde8fe925866e0581b235752354a3f36d5b
Gerrit-Change-Number: 10884
Gerrit-PatchSet: 2
Gerrit-Owner: Tim Armstrong
Gerrit-Reviewer: Tim Armstrong
Gerrit-Reviewer: Todd Lipcon
Gerrit-Comment-Date: Wed, 11 Jul 2018 18:48:45 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7239: Disable smaps parsing by default
Hello Todd Lipcon, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/10884 to look at the new patch set (#3). Change subject: IMPALA-7239: Disable smaps parsing by default .. IMPALA-7239: Disable smaps parsing by default Accessing smaps has proven to be too expensive to enable on all operating systems. Let's move the metrics behind a hidden flag so that they can be enabled for development if needed but won't affect production workloads. Change-Id: I235eddde8fe925866e0581b235752354a3f36d5b --- M be/src/common/init.cc M be/src/util/memory-metrics.cc M be/src/util/process-state-info.cc M be/src/util/process-state-info.h 4 files changed, 66 insertions(+), 46 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/84/10884/3 -- To view, visit http://gerrit.cloudera.org:8080/10884 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I235eddde8fe925866e0581b235752354a3f36d5b Gerrit-Change-Number: 10884 Gerrit-PatchSet: 3 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Todd Lipcon
[Impala-ASF-CR] IMPALA-7239: Disable smaps parsing by default
Todd Lipcon has posted comments on this change. (
http://gerrit.cloudera.org:8080/10884 )
Change subject: IMPALA-7239: Disable smaps parsing by default
..
Patch Set 2:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/10884/1/be/src/util/memory-metrics.cc
File be/src/util/memory-metrics.cc:
http://gerrit.cloudera.org:8080/#/c/10884/1/be/src/util/memory-metrics.cc@124
PS1, Line 124: if (FLAGS_enable_extended_memory_metrics &&
MemInfo::HaveSmaps()) {
> I did some experiments on a CentOS7 system where we saw some perf issues an
that's interesting, thanks for checking.
http://gerrit.cloudera.org:8080/#/c/10884/2/be/src/util/process-state-info.cc
File be/src/util/process-state-info.cc:
http://gerrit.cloudera.org:8080/#/c/10884/2/be/src/util/process-state-info.cc@178
PS2, Line 178: string ProcessStateInfo::DebugString() const {
I dunno where this is used, but would it make sense to not include the bits of
info that weren't read if get_extended_metrics was passed as false? It seems
here they'll all get printed as -1.
--
To view, visit http://gerrit.cloudera.org:8080/10884
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I235eddde8fe925866e0581b235752354a3f36d5b
Gerrit-Change-Number: 10884
Gerrit-PatchSet: 2
Gerrit-Owner: Tim Armstrong
Gerrit-Reviewer: Tim Armstrong
Gerrit-Reviewer: Todd Lipcon
Gerrit-Comment-Date: Fri, 06 Jul 2018 20:59:47 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7239: Disable smaps parsing by default
Tim Armstrong has posted comments on this change. (
http://gerrit.cloudera.org:8080/10884 )
Change subject: IMPALA-7239: Disable smaps parsing by default
..
Patch Set 1:
(3 comments)
I don't think it's a breaking change - it seems reasonable to expect that
metrics collection tools should be able to handle missing data. It's sucky to
add something useful then remove it but seems like the lesser evil at the
moment.
http://gerrit.cloudera.org:8080/#/c/10884/1/be/src/util/memory-metrics.cc
File be/src/util/memory-metrics.cc:
http://gerrit.cloudera.org:8080/#/c/10884/1/be/src/util/memory-metrics.cc@35
PS1, Line 35: enable_all_memory_metrics
> instead of "all" perhaps "smaps" or "extended" or something?
Done
http://gerrit.cloudera.org:8080/#/c/10884/1/be/src/util/memory-metrics.cc@124
PS1, Line 124: AggregateMemoryMetrics::NUM_MAPS =
> if we still care about NUM_MAPS we could parse /proc/self/maps instead of s
I did some experiments on a CentOS7 system where we saw some perf issues and
the sys time to parse /maps is actually about the same as /smaps. I think the
same O(N^2) bug is present because it attributes stacks to threads.
http://gerrit.cloudera.org:8080/#/c/10884/1/be/src/util/memory-metrics.cc@128
PS1, Line 128: AggregateMemoryMetrics::RSS =
aggregate_metrics->AddGauge("memory.rss", 0U);
> RSS is available in /proc/self/status if we care
We already have utilities to parse /status so I switched to using that. I think
a lot of monitoring tools already have good ways to measure RSS and VM size so
those are probably of lesser importance but it's nice having this available
still.
--
To view, visit http://gerrit.cloudera.org:8080/10884
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I235eddde8fe925866e0581b235752354a3f36d5b
Gerrit-Change-Number: 10884
Gerrit-PatchSet: 1
Gerrit-Owner: Tim Armstrong
Gerrit-Reviewer: Tim Armstrong
Gerrit-Reviewer: Todd Lipcon
Gerrit-Comment-Date: Fri, 06 Jul 2018 20:40:08 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7239: Disable smaps parsing by default
Hello Todd Lipcon, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/10884 to look at the new patch set (#2). Change subject: IMPALA-7239: Disable smaps parsing by default .. IMPALA-7239: Disable smaps parsing by default Accessing smaps has proven to be too expensive to enable on all operating systems. Let's move the metrics behind a hidden flag so that they can be enabled for development if needed but won't affect production workloads. Change-Id: I235eddde8fe925866e0581b235752354a3f36d5b --- M be/src/common/init.cc M be/src/util/memory-metrics.cc M be/src/util/process-state-info.cc M be/src/util/process-state-info.h 4 files changed, 31 insertions(+), 16 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/84/10884/2 -- To view, visit http://gerrit.cloudera.org:8080/10884 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I235eddde8fe925866e0581b235752354a3f36d5b Gerrit-Change-Number: 10884 Gerrit-PatchSet: 2 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Todd Lipcon
[Impala-ASF-CR] IMPALA-7239: Disable smaps parsing by default
Todd Lipcon has posted comments on this change. (
http://gerrit.cloudera.org:8080/10884 )
Change subject: IMPALA-7239: Disable smaps parsing by default
..
Patch Set 1:
(3 comments)
Should this be considered incompatible? eg would a downstream metrics program
(eg CM) have problems with us removing these metrics by default?
http://gerrit.cloudera.org:8080/#/c/10884/1/be/src/util/memory-metrics.cc
File be/src/util/memory-metrics.cc:
http://gerrit.cloudera.org:8080/#/c/10884/1/be/src/util/memory-metrics.cc@35
PS1, Line 35: enable_all_memory_metrics
instead of "all" perhaps "smaps" or "extended" or something?
http://gerrit.cloudera.org:8080/#/c/10884/1/be/src/util/memory-metrics.cc@124
PS1, Line 124: AggregateMemoryMetrics::NUM_MAPS =
if we still care about NUM_MAPS we could parse /proc/self/maps instead of
smaps, which I believe is much faster
http://gerrit.cloudera.org:8080/#/c/10884/1/be/src/util/memory-metrics.cc@128
PS1, Line 128: AggregateMemoryMetrics::RSS =
aggregate_metrics->AddGauge("memory.rss", 0U);
RSS is available in /proc/self/status if we care
--
To view, visit http://gerrit.cloudera.org:8080/10884
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I235eddde8fe925866e0581b235752354a3f36d5b
Gerrit-Change-Number: 10884
Gerrit-PatchSet: 1
Gerrit-Owner: Tim Armstrong
Gerrit-Reviewer: Todd Lipcon
Gerrit-Comment-Date: Fri, 06 Jul 2018 19:26:59 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7239: Disable smaps parsing by default
Tim Armstrong has uploaded this change for review. ( http://gerrit.cloudera.org:8080/10884 Change subject: IMPALA-7239: Disable smaps parsing by default .. IMPALA-7239: Disable smaps parsing by default Accessing smaps has proven to be too expensive to enable on all operating systems. Let's move the metrics behind a hidden flag so that they can be enabled for development if needed but won't affect production workloads. Change-Id: I235eddde8fe925866e0581b235752354a3f36d5b --- M be/src/common/init.cc M be/src/util/memory-metrics.cc 2 files changed, 6 insertions(+), 2 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/84/10884/1 -- To view, visit http://gerrit.cloudera.org:8080/10884 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I235eddde8fe925866e0581b235752354a3f36d5b Gerrit-Change-Number: 10884 Gerrit-PatchSet: 1 Gerrit-Owner: Tim Armstrong
