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 <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Joe McDonnell <[email protected]> Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Comment-Date: Tue, 08 Oct 2019 00:31:29 +0000 Gerrit-HasComments: Yes
