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 3: (21 comments) 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@41 PS2, Line 41: /// Returns number of bytes read by this file reader. > I think we always call SetRequestContext() and SetDiskIoMgr() at the same t I modifed the file readers to use the io mgr and request context objects through 'scan_range_'. http://gerrit.cloudera.org:8080/#/c/11444/2/be/src/runtime/io/file-reader.h@50 PS2, Line 50: bytes act > bytes_read() since it's just a trivial accessor Done http://gerrit.cloudera.org:8080/#/c/11444/2/be/src/runtime/io/file-reader.h@81 PS2, Line 81: n_range > this is a plain accessor so prefer 'lock()' Done http://gerrit.cloudera.org:8080/#/c/11444/2/be/src/runtime/io/file-reader.h@93 PS2, Line 93: > const? I don't think this changes after initialization. I changed it to a reference. It can't be a const-ref since we are modifying scan_range_.external_buffer_tag_ in Close(). Also scan_range_.file_string() is a non-const member function. I could create a const version of it, but we pass the returned pointer to functions that take non-const pointers. http://gerrit.cloudera.org:8080/#/c/11444/2/be/src/runtime/io/hdfs-file-reader.h File be/src/runtime/io/hdfs-file-reader.h: http://gerrit.cloudera.org:8080/#/c/11444/2/be/src/runtime/io/hdfs-file-reader.h@47 PS2, Line 47: bool is_borrowed_fh, uint8_t* buffer, int64_t chunk_size, int* bytes_read); > const, since I don't think this is reassigned 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(); > This part was just a mechanical move, right? Would be helpful to flag the p E.g. should I annotate PS1 with gerrit comments? It is the closest PS to the original version, then people could see the diffs relative to PS1. http://gerrit.cloudera.org:8080/#/c/11444/2/be/src/runtime/io/hdfs-file-reader.cc@122 PS2, Line 122: // bytes_read_ is only updated after the while loop > optional: If this part is already touched, it would be nice to extract some Yeah I was thinking about it, but didn't want to do it in the first PS. Done. http://gerrit.cloudera.org:8080/#/c/11444/2/be/src/runtime/io/hdfs-file-reader.cc@132 PS2, Line 132: // - if read was successful (current_bytes_read != -1) > line too long (96 > 90) Done http://gerrit.cloudera.org:8080/#/c/11444/2/be/src/runtime/io/hdfs-file-reader.cc@142 PS2, Line 142: req_context_read_timer.Stop(); > line too long (91 > 90) Done http://gerrit.cloudera.org:8080/#/c/11444/2/be/src/runtime/io/hdfs-file-reader.cc@201 PS2, Line 201: sRead > In the .h Close() is after ReadFromCache(). I would prefer this order in .c Done http://gerrit.cloudera.org:8080/#/c/11444/2/be/src/runtime/io/hdfs-file-reader.cc@203 PS2, Line 203: f (*bytes_r > Variable closed_file looks unnecessary. Done http://gerrit.cloudera.org:8080/#/c/11444/2/be/src/runtime/io/hdfs-file-reader.cc@207 PS2, Line 207: } > line too long (91 > 90) Done http://gerrit.cloudera.org:8080/#/c/11444/2/be/src/runtime/io/hdfs-file-reader.cc@207 PS2, Line 207: > optional: as this block mainly uses private ScanRange variables, I would ex I moved cached_buffer_ into HdfsFileReader since then. For now I leave it as it is. http://gerrit.cloudera.org:8080/#/c/11444/2/be/src/runtime/io/hdfs-file-reader.cc@249 PS2, Line 249: . > I would consider moving (most of) this back to ScanRange::ReadFromCache(). Done http://gerrit.cloudera.org:8080/#/c/11444/2/be/src/runtime/io/hdfs-file-reader.cc@265 PS2, Line 265: << PrettyPrinter::Print(num_remote_bytes_, TUnit::BYTES) > line too long (101 > 90) Done 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 =. Done http://gerrit.cloudera.org:8080/#/c/11444/2/be/src/runtime/io/local-file-reader.h File be/src/runtime/io/local-file-reader.h: http://gerrit.cloudera.org:8080/#/c/11444/2/be/src/runtime/io/local-file-reader.h@40 PS2, Line 40: /// Points to a C FILE object between calls to Open() and Close(), otherwise nullptr. > Maybe briefly document how this fits into the lifecycle, e.g. set in Open() Added comment about it. 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: void* LocalFileReader::CachedFile() { : DCHECK(false); : return nullptr; : } : : S > This looks weird to me - is this ever called? Done http://gerrit.cloudera.org:8080/#/c/11444/2/be/src/runtime/io/local-file-reader.cc@109 PS2, Line 109: = nullptr; > Variable closed_file looks unnecessary. Done http://gerrit.cloudera.org:8080/#/c/11444/2/be/src/runtime/io/request-ranges.h File be/src/runtime/io/request-ranges.h: http://gerrit.cloudera.org:8080/#/c/11444/2/be/src/runtime/io/request-ranges.h@274 PS2, Line 274: int BytesRead() const; > This needs a little more thought - we needed to hold lock_ or scan_range_lo Moved cancel_status_ to the section of 'private members that are accessed by other io:: classes'. The comment was almost already good IMO, only needed some minor changes. http://gerrit.cloudera.org:8080/#/c/11444/2/be/src/runtime/io/request-ranges.h@482 PS2, Line 482: > Comments referencing hdfs_lock_ need updating. Done -- 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: 3 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 15:47:35 +0000 Gerrit-HasComments: Yes
