Qifan Chen has posted comments on this change. ( http://gerrit.cloudera.org:8080/17979 )
Change subject: IMPALA-10791 Add batching reading for remote temporary files ...................................................................... Patch Set 5: (4 comments) http://gerrit.cloudera.org:8080/#/c/17979/5/be/src/runtime/io/disk-io-mgr.cc File be/src/runtime/io/disk-io-mgr.cc: http://gerrit.cloudera.org:8080/#/c/17979/5/be/src/runtime/io/disk-io-mgr.cc@396 PS5, Line 396: DCHECK(disk_file_dst_ != nullptr); : DCHECK(disk_file_src_ != nullptr); May need to add a comment to describe these two files. An example would be helpful too. http://gerrit.cloudera.org:8080/#/c/17979/5/be/src/runtime/io/disk-io-mgr.cc@424 PS5, Line 424: "Mem block '$0' has been deleted, path: '$1'", buffer_idx, remote_file_path)); Why can't we use this DISABLED memory block here? Looks like we should not miss the opportunity of a good use of the memory block. http://gerrit.cloudera.org:8080/#/c/17979/5/be/src/runtime/io/disk-io-mgr.cc@436 PS5, Line 436: ScopedHistogramTimer read_timer(queue->read_latency()); This is not used. http://gerrit.cloudera.org:8080/#/c/17979/5/be/src/runtime/io/disk-io-mgr.cc@445 PS5, Line 445: status = Status(TErrorCode::DISK_IO_ERROR, GetBackendString(), : GetHdfsErrorMsg("Error reading from HDFS file: ", remote_file_path)); Should we set the memory block allocated above to DISABLED here so that it can be used by reading from other offsets? Also, I think we should close the file even in the error reading case. -- To view, visit http://gerrit.cloudera.org:8080/17979 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I1dcc5d0881ffaeff09c5c514306cd668373ad31b Gerrit-Change-Number: 17979 Gerrit-PatchSet: 5 Gerrit-Owner: Yida Wu <wydbaggio...@gmail.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Qifan Chen <qc...@cloudera.com> Gerrit-Reviewer: Yida Wu <wydbaggio...@gmail.com> Gerrit-Comment-Date: Mon, 08 Nov 2021 17:39:26 +0000 Gerrit-HasComments: Yes
