Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/11444 )
Change subject: IMPALA-7556: part 1: handle different file systems via polymorphism ...................................................................... Patch Set 2: (12 comments) http://gerrit.cloudera.org:8080/#/c/11444/3/be/src/runtime/io/file-reader.h File be/src/runtime/io/file-reader.h: http://gerrit.cloudera.org:8080/#/c/11444/3/be/src/runtime/io/file-reader.h@78 PS3, Line 78: // Debug string of this file reader. Optional: switch to SpinLock to reduce overhead on uncontented acquisition path. http://gerrit.cloudera.org:8080/#/c/11444/2/be/src/runtime/io/file-reader.h File be/src/runtime/io/file-reader.h: http://gerrit.cloudera.org:8080/#/c/11444/2/be/src/runtime/io/file-reader.h@93 PS2, Line 93: ScanRange* scan_range_; > I changed it to a reference. I was thinking ScanRange* const scan_range_;. Using a reference is a bit inconsistent with some other parts of the code but I think captures the intent. http://gerrit.cloudera.org:8080/#/c/11444/3/be/src/runtime/io/hdfs-file-reader.h File be/src/runtime/io/hdfs-file-reader.h: http://gerrit.cloudera.org:8080/#/c/11444/3/be/src/runtime/io/hdfs-file-reader.h@51 PS3, Line 51: /// 2. The scan range is using hdfs caching. I should have been clearer - I meant "hdfsFS const hdfs_fs_" since we don't expect the pointer to be reassigned. It's good we can make the pointed-to memory const too. Ok to ignore, this is fine. http://gerrit.cloudera.org:8080/#/c/11444/3/be/src/runtime/io/hdfs-file-reader.h@78 PS3, Line 78: Maybe "non-NULL"? http://gerrit.cloudera.org:8080/#/c/11444/2/be/src/runtime/io/hdfs-file-reader.cc File be/src/runtime/io/hdfs-file-reader.cc: http://gerrit.cloudera.org:8080/#/c/11444/2/be/src/runtime/io/hdfs-file-reader.cc@108 PS2, Line 108: int64_t max_chunk_size = scan_range_->MaxReadChunkSize(); > E.g. should I annotate PS1 with gerrit comments? It is the closest PS to th Yeah if there's a large mechanical component to the changes you made I find it helpful to understand what you did so I can focus on checking the correctness of the mechanical transformation instead of re-reviewing existing code. There's some subtle edge cases in this code so I was mainly wanting to make sure that we preserved them (we should have test cases for them all but I wouldn't bet my life on it). Thomas did something nice with https://gerrit.cloudera.org/#/c/10394/, although that patch was way more unwieldy than this. I sometimes review these things by manually putting the before and after moved code and diffing the two files (although I suspect there are some tools to better automate that that I don't know about yet). I did that for this and I'm basically content now. http://gerrit.cloudera.org:8080/#/c/11444/3/be/src/runtime/io/hdfs-file-reader.cc File be/src/runtime/io/hdfs-file-reader.cc: http://gerrit.cloudera.org:8080/#/c/11444/3/be/src/runtime/io/hdfs-file-reader.cc@171 PS3, Line 171: io_mgr_->ReopenCachedHdfsFileHandle(hdfs_fs_, scan_range_->file_string(), Same comment about local-file-reader for simplifying *eosr logic. http://gerrit.cloudera.org:8080/#/c/11444/3/be/src/runtime/io/hdfs-file-reader.cc@194 PS3, Line 194: return status; I think we can just return the status here, no? We don't need the seek_failed stuff post-refactor. http://gerrit.cloudera.org:8080/#/c/11444/3/be/src/runtime/io/hdfs-file-reader.cc@204 PS3, Line 204: if (exclusive_hdfs_fh_ != nullptr) { We can just return here I think http://gerrit.cloudera.org:8080/#/c/11444/3/be/src/runtime/io/hdfs-file-reader.cc@210 PS3, Line 210: scan_range_->external_buffer_tag_ = ScanRange::ExternalBufferTag::NO_BUFFER; If we return early on the error paths, this just becomes return Status::OK(). http://gerrit.cloudera.org:8080/#/c/11444/3/be/src/runtime/io/local-file-reader.cc File be/src/runtime/io/local-file-reader.cc: http://gerrit.cloudera.org:8080/#/c/11444/3/be/src/runtime/io/local-file-reader.cc@87 PS3, Line 87: } else { We returned on the first branch so we can avoid nested the else branch. http://gerrit.cloudera.org:8080/#/c/11444/3/be/src/runtime/io/local-file-reader.cc@95 PS3, Line 95: if (bytes_read_ == scan_range_->len()) Conditional needs to either be on one line or have parentheses. Or we could actually just rewrite as *eosr = bytes_read_ == scan_range_.len() and avoid initialising above. http://gerrit.cloudera.org:8080/#/c/11444/3/be/src/runtime/io/request-ranges.h File be/src/runtime/io/request-ranges.h: http://gerrit.cloudera.org:8080/#/c/11444/3/be/src/runtime/io/request-ranges.h@339 PS3, Line 339: /// END: private members that are accessed by other io:: classes I also modified this comment in https://gerrit.cloudera.org/#/c/11464/, sorry for the rebase conflict in advance. -- To view, visit http://gerrit.cloudera.org:8080/11444 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ia3d3d2d774075008285230606b992603d5be1a82 Gerrit-Change-Number: 11444 Gerrit-PatchSet: 2 Gerrit-Owner: Zoltan Borok-Nagy <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Reviewer: Zoltan Borok-Nagy <[email protected]> Gerrit-Comment-Date: Wed, 19 Sep 2018 22:22:46 +0000 Gerrit-HasComments: Yes
