You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Tim Armstrong (Code Review)" <ge...@cloudera.org> on 2019/09/17 00:10:27 UTC
[Impala-ASF-CR] IMPALA-8884: track storage read statistics per queue
Tim Armstrong has uploaded this change for review. ( http://gerrit.cloudera.org:8080/14242
Change subject: IMPALA-8884: track storage read statistics per queue
......................................................................
IMPALA-8884: track storage read statistics per queue
Adds the following metrics for each queue:
* impala-server.io-mgr.queue-$i.device-name
* impala-server.io-mgr.queue-$i.read-latency
* impala-server.io-mgr.queue-$i.read-size
I also looked at adding metrics for open operations,
but the plumbing got messy since the code paths
where hdfsOpen() is invoked are more numerous and
complex (e.g. HDFS caching does it outside of a
disk thread).
Perf:
Histograms use atomic operations, so lock contention
isn't an issue.
Ran a TPC-H benchmark on scale factor 30 locally,
saw no perf change.
Change-Id: I8233ed02b418f22f1d0c031e378288357796f4b4
---
M be/src/runtime/exec-env.cc
M be/src/runtime/io/cache-reader-test-stub.h
M be/src/runtime/io/disk-io-mgr-internal.h
M be/src/runtime/io/disk-io-mgr.cc
M be/src/runtime/io/file-reader.h
M be/src/runtime/io/hdfs-file-reader.cc
M be/src/runtime/io/hdfs-file-reader.h
M be/src/runtime/io/local-file-reader.cc
M be/src/runtime/io/local-file-reader.h
M be/src/runtime/io/request-ranges.h
M be/src/runtime/io/scan-range.cc
M be/src/runtime/test-env.cc
M be/src/service/impala-server.cc
M be/src/util/histogram-metric.h
M be/src/util/impalad-metrics.cc
M be/src/util/impalad-metrics.h
M common/thrift/metrics.json
17 files changed, 215 insertions(+), 116 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/42/14242/1
--
To view, visit http://gerrit.cloudera.org:8080/14242
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I8233ed02b418f22f1d0c031e378288357796f4b4
Gerrit-Change-Number: 14242
Gerrit-PatchSet: 1
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-8884: track storage read statistics per queue
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14242 )
Change subject: IMPALA-8884: track storage read statistics per queue
......................................................................
Patch Set 7:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/4753/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/14242
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8233ed02b418f22f1d0c031e378288357796f4b4
Gerrit-Change-Number: 14242
Gerrit-PatchSet: 7
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Tue, 08 Oct 2019 22:28:52 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8884: track storage read statistics per queue
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Hello Michael Ho, Joe McDonnell, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/14242
to look at the new patch set (#7).
Change subject: IMPALA-8884: track storage read statistics per queue
......................................................................
IMPALA-8884: track storage read statistics per queue
Adds the following metrics for each queue:
* impala-server.io-mgr.queue-$i.device-name
* impala-server.io-mgr.queue-$i.read-latency
* impala-server.io-mgr.queue-$i.read-size
I also looked at adding metrics for open operations,
but the plumbing got messy since the code paths
where hdfsOpen() is invoked are more numerous and
complex (e.g. HDFS caching does it outside of a
disk thread).
Perf:
Histograms use atomic operations, so lock contention
isn't an issue.
Ran a TPC-H benchmark on scale factor 30 locally,
saw no perf change.
Change-Id: I8233ed02b418f22f1d0c031e378288357796f4b4
---
M be/src/runtime/exec-env.cc
M be/src/runtime/io/cache-reader-test-stub.h
M be/src/runtime/io/disk-io-mgr-internal.h
M be/src/runtime/io/disk-io-mgr.cc
M be/src/runtime/io/file-reader.h
M be/src/runtime/io/hdfs-file-reader.cc
M be/src/runtime/io/hdfs-file-reader.h
M be/src/runtime/io/local-file-reader.cc
M be/src/runtime/io/local-file-reader.h
M be/src/runtime/io/request-ranges.h
M be/src/runtime/io/scan-range.cc
M be/src/runtime/test-env.cc
M be/src/service/impala-server.cc
M be/src/util/histogram-metric.h
M be/src/util/impalad-metrics.cc
M be/src/util/impalad-metrics.h
M common/thrift/metrics.json
17 files changed, 240 insertions(+), 118 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/42/14242/7
--
To view, visit http://gerrit.cloudera.org:8080/14242
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I8233ed02b418f22f1d0c031e378288357796f4b4
Gerrit-Change-Number: 14242
Gerrit-PatchSet: 7
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-8884: track storage read statistics per queue
Posted by "Joe McDonnell (Code Review)" <ge...@cloudera.org>.
Joe McDonnell has posted comments on this change. ( http://gerrit.cloudera.org:8080/14242 )
Change subject: IMPALA-8884: track storage read statistics per queue
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/14242/2/be/src/runtime/io/hdfs-file-reader.h
File be/src/runtime/io/hdfs-file-reader.h:
http://gerrit.cloudera.org:8080/#/c/14242/2/be/src/runtime/io/hdfs-file-reader.h@73
PS2, Line 73: 'read_latency' is updated with the time spent on filesystem operations.
Comment is stale (argument is now disk_queue)
--
To view, visit http://gerrit.cloudera.org:8080/14242
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8233ed02b418f22f1d0c031e378288357796f4b4
Gerrit-Change-Number: 14242
Gerrit-PatchSet: 2
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Tue, 24 Sep 2019 16:58:18 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8884: track storage read statistics per queue
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/14242 )
Change subject: IMPALA-8884: track storage read statistics per queue
......................................................................
Patch Set 1: Code-Review+1
(4 comments)
http://gerrit.cloudera.org:8080/#/c/14242/1/be/src/runtime/io/disk-io-mgr.cc
File be/src/runtime/io/disk-io-mgr.cc:
http://gerrit.cloudera.org:8080/#/c/14242/1/be/src/runtime/io/disk-io-mgr.cc@278
PS1, Line 278: 1000L * 1000L * 1000L
NANOS_PER_SEC
http://gerrit.cloudera.org:8080/#/c/14242/1/be/src/runtime/io/local-file-reader.h
File be/src/runtime/io/local-file-reader.h:
http://gerrit.cloudera.org:8080/#/c/14242/1/be/src/runtime/io/local-file-reader.h@28
PS1, Line 28: public:
nit: fix the indent here too
http://gerrit.cloudera.org:8080/#/c/14242/1/be/src/runtime/io/local-file-reader.cc
File be/src/runtime/io/local-file-reader.cc:
http://gerrit.cloudera.org:8080/#/c/14242/1/be/src/runtime/io/local-file-reader.cc@64
PS1, Line 64: queue->read_size()->Update(bytes_to_read);
Should we only update read_size() below near where read_timer is in case we hit various errors below ?
http://gerrit.cloudera.org:8080/#/c/14242/1/be/src/runtime/io/local-file-reader.cc@76
PS1, Line 76: Substitute("Could not seek to $0 "
: "for file: $1: $2",
one line
--
To view, visit http://gerrit.cloudera.org:8080/14242
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8233ed02b418f22f1d0c031e378288357796f4b4
Gerrit-Change-Number: 14242
Gerrit-PatchSet: 1
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Comment-Date: Fri, 20 Sep 2019 23:38:01 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8884: track storage read statistics per queue
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Hello Michael Ho, Joe McDonnell, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/14242
to look at the new patch set (#2).
Change subject: IMPALA-8884: track storage read statistics per queue
......................................................................
IMPALA-8884: track storage read statistics per queue
Adds the following metrics for each queue:
* impala-server.io-mgr.queue-$i.device-name
* impala-server.io-mgr.queue-$i.read-latency
* impala-server.io-mgr.queue-$i.read-size
I also looked at adding metrics for open operations,
but the plumbing got messy since the code paths
where hdfsOpen() is invoked are more numerous and
complex (e.g. HDFS caching does it outside of a
disk thread).
Perf:
Histograms use atomic operations, so lock contention
isn't an issue.
Ran a TPC-H benchmark on scale factor 30 locally,
saw no perf change.
Change-Id: I8233ed02b418f22f1d0c031e378288357796f4b4
---
M be/src/runtime/exec-env.cc
M be/src/runtime/io/cache-reader-test-stub.h
M be/src/runtime/io/disk-io-mgr-internal.h
M be/src/runtime/io/disk-io-mgr.cc
M be/src/runtime/io/file-reader.h
M be/src/runtime/io/hdfs-file-reader.cc
M be/src/runtime/io/hdfs-file-reader.h
M be/src/runtime/io/local-file-reader.cc
M be/src/runtime/io/local-file-reader.h
M be/src/runtime/io/request-ranges.h
M be/src/runtime/io/scan-range.cc
M be/src/runtime/test-env.cc
M be/src/service/impala-server.cc
M be/src/util/histogram-metric.h
M be/src/util/impalad-metrics.cc
M be/src/util/impalad-metrics.h
M common/thrift/metrics.json
17 files changed, 216 insertions(+), 118 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/42/14242/2
--
To view, visit http://gerrit.cloudera.org:8080/14242
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I8233ed02b418f22f1d0c031e378288357796f4b4
Gerrit-Change-Number: 14242
Gerrit-PatchSet: 2
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
[Impala-ASF-CR] IMPALA-8884: track storage read statistics per queue
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14242 )
Change subject: IMPALA-8884: track storage read statistics per queue
......................................................................
Patch Set 1:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/4571/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/14242
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8233ed02b418f22f1d0c031e378288357796f4b4
Gerrit-Change-Number: 14242
Gerrit-PatchSet: 1
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Comment-Date: Tue, 17 Sep 2019 00:51:10 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8884: track storage read statistics per queue
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14242 )
Change subject: IMPALA-8884: track storage read statistics per queue
......................................................................
Patch Set 8:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/5072/ DRY_RUN=false
--
To view, visit http://gerrit.cloudera.org:8080/14242
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8233ed02b418f22f1d0c031e378288357796f4b4
Gerrit-Change-Number: 14242
Gerrit-PatchSet: 8
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Tue, 08 Oct 2019 22:12:22 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8884: track storage read statistics per queue
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14242 )
Change subject: IMPALA-8884: track storage read statistics per queue
......................................................................
Patch Set 5:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/4743/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/14242
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8233ed02b418f22f1d0c031e378288357796f4b4
Gerrit-Change-Number: 14242
Gerrit-PatchSet: 5
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Tue, 08 Oct 2019 01:37:42 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8884: track storage read statistics per queue
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/14242 )
Change subject: IMPALA-8884: track storage read statistics per queue
......................................................................
IMPALA-8884: track storage read statistics per queue
Adds the following metrics for each queue:
* impala-server.io-mgr.queue-$i.device-name
* impala-server.io-mgr.queue-$i.read-latency
* impala-server.io-mgr.queue-$i.read-size
I also looked at adding metrics for open operations,
but the plumbing got messy since the code paths
where hdfsOpen() is invoked are more numerous and
complex (e.g. HDFS caching does it outside of a
disk thread).
Perf:
Histograms use atomic operations, so lock contention
isn't an issue.
Ran a TPC-H benchmark on scale factor 30 locally,
saw no perf change.
Change-Id: I8233ed02b418f22f1d0c031e378288357796f4b4
Reviewed-on: http://gerrit.cloudera.org:8080/14242
Reviewed-by: Impala Public Jenkins <im...@cloudera.com>
Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
M be/src/runtime/exec-env.cc
M be/src/runtime/io/cache-reader-test-stub.h
M be/src/runtime/io/disk-io-mgr-internal.h
M be/src/runtime/io/disk-io-mgr.cc
M be/src/runtime/io/file-reader.h
M be/src/runtime/io/hdfs-file-reader.cc
M be/src/runtime/io/hdfs-file-reader.h
M be/src/runtime/io/local-file-reader.cc
M be/src/runtime/io/local-file-reader.h
M be/src/runtime/io/request-ranges.h
M be/src/runtime/io/scan-range.cc
M be/src/runtime/test-env.cc
M be/src/service/impala-server.cc
M be/src/util/histogram-metric.h
M be/src/util/impalad-metrics.cc
M be/src/util/impalad-metrics.h
M common/thrift/metrics.json
17 files changed, 240 insertions(+), 118 deletions(-)
Approvals:
Impala Public Jenkins: Looks good to me, approved; Verified
--
To view, visit http://gerrit.cloudera.org:8080/14242
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I8233ed02b418f22f1d0c031e378288357796f4b4
Gerrit-Change-Number: 14242
Gerrit-PatchSet: 9
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-8884: track storage read statistics per queue
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/14242 )
Change subject: IMPALA-8884: track storage read statistics per queue
......................................................................
Patch Set 6: Code-Review+1
carry +1
--
To view, visit http://gerrit.cloudera.org:8080/14242
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8233ed02b418f22f1d0c031e378288357796f4b4
Gerrit-Change-Number: 14242
Gerrit-PatchSet: 6
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Tue, 08 Oct 2019 00:53:47 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8884: track storage read statistics per queue
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/14242 )
Change subject: IMPALA-8884: track storage read statistics per queue
......................................................................
Patch Set 1:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/14242/1/be/src/runtime/io/disk-io-mgr.cc
File be/src/runtime/io/disk-io-mgr.cc:
http://gerrit.cloudera.org:8080/#/c/14242/1/be/src/runtime/io/disk-io-mgr.cc@278
PS1, Line 278: 1000L * 1000L * 1000L
> NANOS_PER_SEC
Done
http://gerrit.cloudera.org:8080/#/c/14242/1/be/src/runtime/io/local-file-reader.h
File be/src/runtime/io/local-file-reader.h:
http://gerrit.cloudera.org:8080/#/c/14242/1/be/src/runtime/io/local-file-reader.h@28
PS1, Line 28: public:
> nit: fix the indent here too
Done
http://gerrit.cloudera.org:8080/#/c/14242/1/be/src/runtime/io/local-file-reader.cc
File be/src/runtime/io/local-file-reader.cc:
http://gerrit.cloudera.org:8080/#/c/14242/1/be/src/runtime/io/local-file-reader.cc@64
PS1, Line 64: queue->read_size()->Update(bytes_to_read);
> Should we only update read_size() below near where read_timer is in case we
Done
http://gerrit.cloudera.org:8080/#/c/14242/1/be/src/runtime/io/local-file-reader.cc@76
PS1, Line 76: Substitute("Could not seek to $0 "
: "for file: $1: $2",
> one line
Done
--
To view, visit http://gerrit.cloudera.org:8080/14242
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8233ed02b418f22f1d0c031e378288357796f4b4
Gerrit-Change-Number: 14242
Gerrit-PatchSet: 1
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Tue, 24 Sep 2019 16:53:20 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8884: track storage read statistics per queue
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/14242 )
Change subject: IMPALA-8884: track storage read statistics per queue
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/14242/2/be/src/runtime/io/hdfs-file-reader.h
File be/src/runtime/io/hdfs-file-reader.h:
http://gerrit.cloudera.org:8080/#/c/14242/2/be/src/runtime/io/hdfs-file-reader.h@73
PS2, Line 73: 'read_latency' is updated with the time spent on filesystem operations.
> Comment is stale (argument is now disk_queue)
Done
--
To view, visit http://gerrit.cloudera.org:8080/14242
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8233ed02b418f22f1d0c031e378288357796f4b4
Gerrit-Change-Number: 14242
Gerrit-PatchSet: 2
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Tue, 24 Sep 2019 21:06:40 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8884: track storage read statistics per queue
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/14242 )
Change subject: IMPALA-8884: track storage read statistics per queue
......................................................................
Patch Set 3:
It turns out the initial patchset was missing some fixes required to get DiskIoMgr to work correctly in unit tests - it was double-registering metrics and hitting a DCHECK
--
To view, visit http://gerrit.cloudera.org:8080/14242
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8233ed02b418f22f1d0c031e378288357796f4b4
Gerrit-Change-Number: 14242
Gerrit-PatchSet: 3
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Tue, 24 Sep 2019 17:21:38 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8884: track storage read statistics per queue
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14242 )
Change subject: IMPALA-8884: track storage read statistics per queue
......................................................................
Patch Set 2:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/4627/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/14242
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8233ed02b418f22f1d0c031e378288357796f4b4
Gerrit-Change-Number: 14242
Gerrit-PatchSet: 2
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Tue, 24 Sep 2019 17:30:41 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8884: track storage read statistics per queue
Posted by "Joe McDonnell (Code Review)" <ge...@cloudera.org>.
Joe McDonnell has posted comments on this change. ( http://gerrit.cloudera.org:8080/14242 )
Change subject: IMPALA-8884: track storage read statistics per queue
......................................................................
Patch Set 6:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/14242/6/be/src/runtime/io/disk-io-mgr.cc
File be/src/runtime/io/disk-io-mgr.cc:
http://gerrit.cloudera.org:8080/#/c/14242/6/be/src/runtime/io/disk-io-mgr.cc@142
PS6, Line 142: // This parameter controls whether remote HDFS file handles are cached. It does not impact
: // S3, ADLS, or ABFS file handles.
: DEFINE_bool(cache_remote_file_handles, true,
: "Enable the file handle cache for "
: "remote HDFS files.");
:
: // This parameter controls whether S3 file handles are cached.
: DEFINE_bool(cache_s3_file_handles, true,
: "Enable the file handle cache for "
: "S3 files.");
The formatting fix didn't make it.
http://gerrit.cloudera.org:8080/#/c/14242/6/be/src/runtime/io/disk-io-mgr.cc@289
PS6, Line 289: if (TestInfo::is_test()
: && ImpaladMetrics::IO_MGR_METRICS->FindMetricForTesting<HistogramMetric>(
: Substitute(DEVICE_NAME_METRIC_KEY_TEMPLATE, i_string))
: == nullptr) {
Couple things:
1. I think this wouldn't set the device name outside of a test, because we would never do the AddProperty command. I think you want something like:
bool device_name_set = false;
if (TestInfo::is_test()) {
device_name_set = (FindMetricForTesting() == null);
}
if (!device_name_set) {
AddProperty();
}
(Or store the actual property pointer)
2. What is the right type for a property? We are using FindMetricForTesting with a HistogramMetric for a property. It's only for existence checking, but is there a better template type to use for a property?
--
To view, visit http://gerrit.cloudera.org:8080/14242
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8233ed02b418f22f1d0c031e378288357796f4b4
Gerrit-Change-Number: 14242
Gerrit-PatchSet: 6
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Tue, 08 Oct 2019 21:08:01 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8884: track storage read statistics per queue
Posted by "Joe McDonnell (Code Review)" <ge...@cloudera.org>.
Joe McDonnell has posted comments on this change. ( http://gerrit.cloudera.org:8080/14242 )
Change subject: IMPALA-8884: track storage read statistics per queue
......................................................................
Patch Set 7: Code-Review+2
This looks good to me. Michael +1'd before, so I'm going to +2 this.
--
To view, visit http://gerrit.cloudera.org:8080/14242
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8233ed02b418f22f1d0c031e378288357796f4b4
Gerrit-Change-Number: 14242
Gerrit-PatchSet: 7
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Tue, 08 Oct 2019 21:59:59 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8884: track storage read statistics per queue
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Hello Michael Ho, Joe McDonnell, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/14242
to look at the new patch set (#3).
Change subject: IMPALA-8884: track storage read statistics per queue
......................................................................
IMPALA-8884: track storage read statistics per queue
Adds the following metrics for each queue:
* impala-server.io-mgr.queue-$i.device-name
* impala-server.io-mgr.queue-$i.read-latency
* impala-server.io-mgr.queue-$i.read-size
I also looked at adding metrics for open operations,
but the plumbing got messy since the code paths
where hdfsOpen() is invoked are more numerous and
complex (e.g. HDFS caching does it outside of a
disk thread).
Perf:
Histograms use atomic operations, so lock contention
isn't an issue.
Ran a TPC-H benchmark on scale factor 30 locally,
saw no perf change.
Change-Id: I8233ed02b418f22f1d0c031e378288357796f4b4
---
M be/src/runtime/exec-env.cc
M be/src/runtime/io/cache-reader-test-stub.h
M be/src/runtime/io/disk-io-mgr-internal.h
M be/src/runtime/io/disk-io-mgr.cc
M be/src/runtime/io/file-reader.h
M be/src/runtime/io/hdfs-file-reader.cc
M be/src/runtime/io/hdfs-file-reader.h
M be/src/runtime/io/local-file-reader.cc
M be/src/runtime/io/local-file-reader.h
M be/src/runtime/io/request-ranges.h
M be/src/runtime/io/scan-range.cc
M be/src/runtime/test-env.cc
M be/src/service/impala-server.cc
M be/src/util/histogram-metric.h
M be/src/util/impalad-metrics.cc
M be/src/util/impalad-metrics.h
M common/thrift/metrics.json
17 files changed, 237 insertions(+), 120 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/42/14242/3
--
To view, visit http://gerrit.cloudera.org:8080/14242
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I8233ed02b418f22f1d0c031e378288357796f4b4
Gerrit-Change-Number: 14242
Gerrit-PatchSet: 3
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-8884: track storage read statistics per queue
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Hello Michael Ho, Joe McDonnell, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/14242
to look at the new patch set (#5).
Change subject: IMPALA-8884: track storage read statistics per queue
......................................................................
IMPALA-8884: track storage read statistics per queue
Adds the following metrics for each queue:
* impala-server.io-mgr.queue-$i.device-name
* impala-server.io-mgr.queue-$i.read-latency
* impala-server.io-mgr.queue-$i.read-size
I also looked at adding metrics for open operations,
but the plumbing got messy since the code paths
where hdfsOpen() is invoked are more numerous and
complex (e.g. HDFS caching does it outside of a
disk thread).
Perf:
Histograms use atomic operations, so lock contention
isn't an issue.
Ran a TPC-H benchmark on scale factor 30 locally,
saw no perf change.
Change-Id: I8233ed02b418f22f1d0c031e378288357796f4b4
---
M be/src/runtime/exec-env.cc
M be/src/runtime/io/cache-reader-test-stub.h
M be/src/runtime/io/disk-io-mgr-internal.h
M be/src/runtime/io/disk-io-mgr.cc
M be/src/runtime/io/file-reader.h
M be/src/runtime/io/hdfs-file-reader.cc
M be/src/runtime/io/hdfs-file-reader.h
M be/src/runtime/io/local-file-reader.cc
M be/src/runtime/io/local-file-reader.h
M be/src/runtime/io/request-ranges.h
M be/src/runtime/io/scan-range.cc
M be/src/runtime/test-env.cc
M be/src/service/impala-server.cc
M be/src/util/histogram-metric.h
M be/src/util/impalad-metrics.cc
M be/src/util/impalad-metrics.h
M common/thrift/metrics.json
17 files changed, 244 insertions(+), 120 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/42/14242/5
--
To view, visit http://gerrit.cloudera.org:8080/14242
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I8233ed02b418f22f1d0c031e378288357796f4b4
Gerrit-Change-Number: 14242
Gerrit-PatchSet: 5
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-8884: track storage read statistics per queue
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Hello Michael Ho, Joe McDonnell, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/14242
to look at the new patch set (#4).
Change subject: IMPALA-8884: track storage read statistics per queue
......................................................................
IMPALA-8884: track storage read statistics per queue
Adds the following metrics for each queue:
* impala-server.io-mgr.queue-$i.device-name
* impala-server.io-mgr.queue-$i.read-latency
* impala-server.io-mgr.queue-$i.read-size
I also looked at adding metrics for open operations,
but the plumbing got messy since the code paths
where hdfsOpen() is invoked are more numerous and
complex (e.g. HDFS caching does it outside of a
disk thread).
Perf:
Histograms use atomic operations, so lock contention
isn't an issue.
Ran a TPC-H benchmark on scale factor 30 locally,
saw no perf change.
Change-Id: I8233ed02b418f22f1d0c031e378288357796f4b4
---
M be/src/runtime/exec-env.cc
M be/src/runtime/io/cache-reader-test-stub.h
M be/src/runtime/io/disk-io-mgr-internal.h
M be/src/runtime/io/disk-io-mgr.cc
M be/src/runtime/io/file-reader.h
M be/src/runtime/io/hdfs-file-reader.cc
M be/src/runtime/io/hdfs-file-reader.h
M be/src/runtime/io/local-file-reader.cc
M be/src/runtime/io/local-file-reader.h
M be/src/runtime/io/request-ranges.h
M be/src/runtime/io/scan-range.cc
M be/src/runtime/test-env.cc
M be/src/service/impala-server.cc
M be/src/util/histogram-metric.h
M be/src/util/impalad-metrics.cc
M be/src/util/impalad-metrics.h
M common/thrift/metrics.json
17 files changed, 237 insertions(+), 120 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/42/14242/4
--
To view, visit http://gerrit.cloudera.org:8080/14242
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I8233ed02b418f22f1d0c031e378288357796f4b4
Gerrit-Change-Number: 14242
Gerrit-PatchSet: 4
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-8884: track storage read statistics per queue
Posted by "Joe McDonnell (Code Review)" <ge...@cloudera.org>.
Joe McDonnell has posted comments on this change. ( http://gerrit.cloudera.org:8080/14242 )
Change subject: IMPALA-8884: track storage read statistics per queue
......................................................................
Patch Set 4: Code-Review+1
(3 comments)
I think this makes sense.
http://gerrit.cloudera.org:8080/#/c/14242/4/be/src/runtime/io/disk-io-mgr.cc
File be/src/runtime/io/disk-io-mgr.cc:
http://gerrit.cloudera.org:8080/#/c/14242/4/be/src/runtime/io/disk-io-mgr.cc@148
PS4, Line 148: DEFINE_bool(cache_s3_file_handles, true,
: "Enable the file handle cache for "
: "S3 files.");
Nit: There are some edge cases the code formatter doesn't handle. This one and the cache_remote_file_handles comment should have the strings consolidated on one line.
http://gerrit.cloudera.org:8080/#/c/14242/4/be/src/runtime/io/disk-io-mgr.cc@288
PS4, Line 288: FindMetricForTesting
This is a pretty uncommon case where tests register metrics twice, so it probably doesn't merit major changes to the interfaces. We could drop the "ForTesting" part of the name. I don't think I have very strong feelings about that.
http://gerrit.cloudera.org:8080/#/c/14242/4/be/src/runtime/io/disk-io-mgr.cc@294
PS4, Line 294: HistogramMetric* read_latency =
: ImpaladMetrics::IO_MGR_METRICS->FindMetricForTesting<HistogramMetric>(
: Substitute(READ_LATENCY_METRIC_KEY_TEMPLATE, i_string));
: disk_queues_[i]->set_read_latency(read_latency != nullptr ? read_latency :
: ImpaladMetrics::IO_MGR_METRICS->RegisterMetric(new HistogramMetric(
: MetricDefs::Get(READ_LATENCY_METRIC_KEY_TEMPLATE, i_string),
: ONE_HOUR_IN_NS, 3)));
For these cases, we expect read_latency to be null unless this is a backend test, so we could add a DCHECK like:
DCHECK(read_latency == nullptr || TestInfo::is_be_test());
Same for read_size.
--
To view, visit http://gerrit.cloudera.org:8080/14242
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8233ed02b418f22f1d0c031e378288357796f4b4
Gerrit-Change-Number: 14242
Gerrit-PatchSet: 4
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Tue, 08 Oct 2019 00:31:29 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8884: track storage read statistics per queue
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14242 )
Change subject: IMPALA-8884: track storage read statistics per queue
......................................................................
Patch Set 8: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/14242
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8233ed02b418f22f1d0c031e378288357796f4b4
Gerrit-Change-Number: 14242
Gerrit-PatchSet: 8
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Tue, 08 Oct 2019 22:12:21 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8884: track storage read statistics per queue
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14242 )
Change subject: IMPALA-8884: track storage read statistics per queue
......................................................................
Patch Set 4:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/4630/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/14242
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8233ed02b418f22f1d0c031e378288357796f4b4
Gerrit-Change-Number: 14242
Gerrit-PatchSet: 4
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Tue, 24 Sep 2019 21:49:59 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8884: track storage read statistics per queue
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14242 )
Change subject: IMPALA-8884: track storage read statistics per queue
......................................................................
Patch Set 3:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/4628/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/14242
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8233ed02b418f22f1d0c031e378288357796f4b4
Gerrit-Change-Number: 14242
Gerrit-PatchSet: 3
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Tue, 24 Sep 2019 18:01:56 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8884: track storage read statistics per queue
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14242 )
Change subject: IMPALA-8884: track storage read statistics per queue
......................................................................
Patch Set 8: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/14242
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8233ed02b418f22f1d0c031e378288357796f4b4
Gerrit-Change-Number: 14242
Gerrit-PatchSet: 8
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Wed, 09 Oct 2019 02:29:01 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8884: track storage read statistics per queue
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/14242 )
Change subject: IMPALA-8884: track storage read statistics per queue
......................................................................
Patch Set 6:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/14242/6/be/src/runtime/io/disk-io-mgr.cc
File be/src/runtime/io/disk-io-mgr.cc:
http://gerrit.cloudera.org:8080/#/c/14242/6/be/src/runtime/io/disk-io-mgr.cc@142
PS6, Line 142: // This parameter controls whether remote HDFS file handles are cached. It does not impact
: // S3, ADLS, or ABFS file handles.
: DEFINE_bool(cache_remote_file_handles, true,
: "Enable the file handle cache for "
: "remote HDFS files.");
:
: // This parameter controls whether S3 file handles are cached.
: DEFINE_bool(cache_s3_file_handles, true,
: "Enable the file handle cache for "
: "S3 files.");
> The formatting fix didn't make it.
Done
http://gerrit.cloudera.org:8080/#/c/14242/6/be/src/runtime/io/disk-io-mgr.cc@289
PS6, Line 289: if (TestInfo::is_test()
: && ImpaladMetrics::IO_MGR_METRICS->FindMetricForTesting<HistogramMetric>(
: Substitute(DEVICE_NAME_METRIC_KEY_TEMPLATE, i_string))
: == nullptr) {
> Couple things:
Both good points. I restructured the conditional to be correct and confirmed the metric is created. Switched to StringProperty too.
--
To view, visit http://gerrit.cloudera.org:8080/14242
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8233ed02b418f22f1d0c031e378288357796f4b4
Gerrit-Change-Number: 14242
Gerrit-PatchSet: 6
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Tue, 08 Oct 2019 21:48:20 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8884: track storage read statistics per queue
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/14242 )
Change subject: IMPALA-8884: track storage read statistics per queue
......................................................................
Patch Set 4:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/14242/4/be/src/runtime/io/disk-io-mgr.cc
File be/src/runtime/io/disk-io-mgr.cc:
http://gerrit.cloudera.org:8080/#/c/14242/4/be/src/runtime/io/disk-io-mgr.cc@148
PS4, Line 148: DEFINE_bool(cache_s3_file_handles, true,
: "Enable the file handle cache for "
: "S3 files.");
> Nit: There are some edge cases the code formatter doesn't handle. This one
Done - reverted to old formatting.
http://gerrit.cloudera.org:8080/#/c/14242/4/be/src/runtime/io/disk-io-mgr.cc@288
PS4, Line 288: FindMetricForTesting
> This is a pretty uncommon case where tests register metrics twice, so it pr
I added an is_test() check beforehand so that this is only called when running in a test environment.
http://gerrit.cloudera.org:8080/#/c/14242/4/be/src/runtime/io/disk-io-mgr.cc@294
PS4, Line 294: HistogramMetric* read_latency =
: ImpaladMetrics::IO_MGR_METRICS->FindMetricForTesting<HistogramMetric>(
: Substitute(READ_LATENCY_METRIC_KEY_TEMPLATE, i_string));
: disk_queues_[i]->set_read_latency(read_latency != nullptr ? read_latency :
: ImpaladMetrics::IO_MGR_METRICS->RegisterMetric(new HistogramMetric(
: MetricDefs::Get(READ_LATENCY_METRIC_KEY_TEMPLATE, i_string),
: ONE_HOUR_IN_NS, 3)));
> For these cases, we expect read_latency to be null unless this is a backend
I changed it around a bit so that the FindMetricForTesting() code only runs as part of tests.
--
To view, visit http://gerrit.cloudera.org:8080/14242
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8233ed02b418f22f1d0c031e378288357796f4b4
Gerrit-Change-Number: 14242
Gerrit-PatchSet: 4
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Tue, 08 Oct 2019 00:53:27 +0000
Gerrit-HasComments: Yes