Csaba Ringhofer 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: (8 comments) I have minimal experience with IO manager so I may not understand the intent of the refactor in some places. 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@122 PS2, Line 122: while (true) { optional: If this part is already touched, it would be nice to extract some parts to functions to reduce function length/nestedness. At the first glance I would try to extract the part that is "retried" to a function like Status ReadFromPosInternal(...lot of args). http://gerrit.cloudera.org:8080/#/c/11444/2/be/src/runtime/io/hdfs-file-reader.cc@201 PS2, Line 201: Close In the .h Close() is after ReadFromCache(). I would prefer this order in .cc to be consistent and also to group read functions together. http://gerrit.cloudera.org:8080/#/c/11444/2/be/src/runtime/io/hdfs-file-reader.cc@203 PS2, Line 203: closed_file Variable closed_file looks unnecessary. http://gerrit.cloudera.org:8080/#/c/11444/2/be/src/runtime/io/hdfs-file-reader.cc@207 PS2, Line 207: if optional: as this block mainly uses private ScanRange variables, I would extract it to a function in ScanRange like FreeCachedExternalBuffer(). http://gerrit.cloudera.org:8080/#/c/11444/2/be/src/runtime/io/hdfs-file-reader.cc@249 PS2, Line 249: ReadFromCache I would consider moving (most of) this back to ScanRange::ReadFromCache(). The local file version doesn't seem meaningful + this uses scan_range's private variables much more often than HDFSFileReader's. http://gerrit.cloudera.org:8080/#/c/11444/2/be/src/runtime/io/hdfs-file-reader.cc@334 PS2, Line 334: nit: other members have no space after =. http://gerrit.cloudera.org:8080/#/c/11444/2/be/src/runtime/io/local-file-reader.cc File be/src/runtime/io/local-file-reader.cc: http://gerrit.cloudera.org:8080/#/c/11444/2/be/src/runtime/io/local-file-reader.cc@100 PS2, Line 100: Status LocalFileReader::ReadFromCache(const unique_lock<mutex>& reader_lock, : bool* read_succeeded) { : DCHECK(reader_lock.mutex() == &request_context_->lock_ && reader_lock.owns_lock()); : DCHECK_EQ(bytes_read_, 0); : return Status::OK(); : } This looks weird to me - is this ever called? I checked the callsites of ReadFromCache() and read_succeeded is not initialized before the call, so function will also leave it in an uninitialized state. If this should never be called, then I would replace add DCHECK(false). http://gerrit.cloudera.org:8080/#/c/11444/2/be/src/runtime/io/local-file-reader.cc@109 PS2, Line 109: closed_file Variable closed_file looks unnecessary. -- 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-Comment-Date: Mon, 17 Sep 2018 14:32:32 +0000 Gerrit-HasComments: Yes
