Joe McDonnell 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 1: (2 comments) Thanks for working on this. This is looking good to me. Just a couple style nits. I'm thinking about whether there are any tests I want you to run. I'll get back to you. 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: CachedHdfsFileHandle* borrowed_hdfs_fh = nullptr; : hdfsFile hdfs_file; : : // Make sure to release any borrowed file handle. : auto release_borrowed_hdfs_fh = MakeScopeExitTrigger([this, &borrowed_hdfs_fh]() { : if (borrowed_hdfs_fh != nullptr) { : scan_range_->io_mgr_->ReleaseCachedHdfsFileHandle(scan_range_->file_string(), : borrowed_hdfs_fh); : } : }); Nit: Since these are now initialized later in the function, I would also move the definitions of the variables down to just before the "if (*bytes_read < bytes_to_read)" statement. I think I would also move the scope exit trigger down to just after that if block (just before the while). http://gerrit.cloudera.org:8080/#/c/16963/1/be/src/runtime/io/hdfs-file-reader.cc@126 PS1, Line 126: if (*bytes_read < bytes_to_read) { For the clang-tidy issue you are seeing here: https://jenkins.impala.io/job/clang-tidy-ub1604/14691/artifact/tidylog.txt clang-tidy must not be smart enough to know that this if statement has a condition that is the same as the while loop, so it thinks hdfs_file is uninitialized. One option is to bail out early here if *bytes_read == bytes_to_read, then just have these variables without an if statement. Looking through the logic later in the function, none of it would run in that case, so I think it is safe to bail. (Obviously, we aren't reading anything more, and the block that does WriteDataCache() also doesn't have anything to do.) -- 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: 1 Gerrit-Owner: Riza Suminto <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Joe McDonnell <[email protected]> Gerrit-Reviewer: Riza Suminto <[email protected]> Gerrit-Reviewer: Zoltan Borok-Nagy <[email protected]> Gerrit-Comment-Date: Thu, 21 Jan 2021 00:21:33 +0000 Gerrit-HasComments: Yes
