Zoltan Borok-Nagy 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 4: (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: SpinLock lock_; > Optional: switch to SpinLock to reduce overhead on uncontented acquisition Done 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: > I was thinking ScanRange* const scan_range_;. Using a reference is a bit in Ok, switched to ScanRange* for consistency. 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: hdfsFS const hdfs_fs_; > I should have been clearer - I meant "hdfsFS const hdfs_fs_" since we don't Actually, since hdfsFS is a typedef it means the same: https://stackoverflow.com/questions/8504411/typedef-pointer-const-weirdness Anyway, I swapped the two words to cease confusion. The hdfs api functions expect non-const pointed-to-memory as far as I can tell, so we can't really make the pointed-to memory const. http://gerrit.cloudera.org:8080/#/c/11444/3/be/src/runtime/io/hdfs-file-reader.h@78 PS3, Line 78: Non-N > Maybe "non-NULL"? Done 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: hdfs_file = borrowed_hdfs_fh->file(); > Yeah if there's a large mechanical component to the changes you made I find Yeah it is hard to do it in a diff-friendly way and I'm also not aware of any good tools that can show the moved code parts. OK, I'll create another gerrit reply about PS1 and comment the origin of the copy-pasted parts so later reviewers might find it useful. 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: if (bytes_read_ == scan_range_->len()) *eosr = true; > Same comment about local-file-reader for simplifying *eosr logic. I have the same concern. At L154 we set *eosr to true if the scan range went past the end of the file. We might not want to set it to false here. Again, I'm not sure if it is a valid concern, but since I don't see DCHECKs against it I suppose it is valid. http://gerrit.cloudera.org:8080/#/c/11444/3/be/src/runtime/io/hdfs-file-reader.cc@194 PS3, Line 194: Substitute("Error seeking to $0 in file: $1: $2", > I think we can just return the status here, no? We don't need the seek_fail True, done! http://gerrit.cloudera.org:8080/#/c/11444/3/be/src/runtime/io/hdfs-file-reader.cc@204 PS3, Line 204: } > We can just return here I think Done http://gerrit.cloudera.org:8080/#/c/11444/3/be/src/runtime/io/hdfs-file-reader.cc@210 PS3, Line 210: { > If we return early on the error paths, this just becomes return Status::OK( Done 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: } > We returned on the first branch so we can avoid nested the else branch. Done http://gerrit.cloudera.org:8080/#/c/11444/3/be/src/runtime/io/local-file-reader.cc@95 PS3, Line 95: return Status::OK(); > Conditional needs to either be on one line or have parentheses. Or we could We set *eosr to true at L90 when we reach eof, so probably we don't want to set it to false when bytes_read_ != scan_range_.len(). However, I don't really know if it's possible to have a scan range that runs over the end of the file. Maybe if we had open-ended scan ranges for reading everything from a given position until the end of file. Around L90 there are no DCHECKs like DCHECK_EQ(bytes_read_, scan_range_.len()), so I suppose it is possible. 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: /// cancellation - CANCELLED if cancelled without error, or another status if an > I also modified this comment in https://gerrit.cloudera.org/#/c/11464/, sor It's OK, thanks for notifying I'll watch for this during the rebase. -- 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: 4 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: Thu, 20 Sep 2018 10:37:45 +0000 Gerrit-HasComments: Yes
