Riza Suminto has posted comments on this change. ( http://gerrit.cloudera.org:8080/16963 )
Change subject: IMPALA-10147: Avoid getting a file handle for data cache hits ...................................................................... Patch Set 2: (2 comments) Thank you for the review. Patch set 2 move the handle variable and scope exit trigger closer around the file handle retrieval. http://gerrit.cloudera.org:8080/#/c/16963/1/be/src/runtime/io/hdfs-file-reader.cc File be/src/runtime/io/hdfs-file-reader.cc: http://gerrit.cloudera.org:8080/#/c/16963/1/be/src/runtime/io/hdfs-file-reader.cc@99 PS1, Line 99: Status status = Status::OK(); : { : ScopedTimer<MonotonicStopWatch> req_context_read_timer( : scan_range_->reader_->read_timer_); : bool logged_slow_read = false; // True if we already logged about a slow read. : : // Try reading from the remote data cache if it's enabled for the scan range. : DataCache* remote_data_cache = io_mgr->remote_data_cache(); : bool try_data_cache = scan_range_->UseDataCache() && remote_data_cache != nullptr; : i > Nit: Since these are now initialized later in the function, I would also mo Done http://gerrit.cloudera.org:8080/#/c/16963/1/be/src/runtime/io/hdfs-file-reader.cc@126 PS1, Line 126: hdfsFile hdfs_file; > For the clang-tidy issue you are seeing here: Thank you. I didn't read thoroughly to realize that the logic later won't execute if all bytes successfully read from data cache. -- To view, visit http://gerrit.cloudera.org:8080/16963 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Icc68f233518f862454e87bcbbef14d65fcdb7c91 Gerrit-Change-Number: 16963 Gerrit-PatchSet: 2 Gerrit-Owner: Riza Suminto <riza.sumi...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Joe McDonnell <joemcdonn...@cloudera.com> Gerrit-Reviewer: Riza Suminto <riza.sumi...@cloudera.com> Gerrit-Reviewer: Zoltan Borok-Nagy <borokna...@cloudera.com> Gerrit-Comment-Date: Thu, 21 Jan 2021 17:13:52 +0000 Gerrit-HasComments: Yes
