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 5: (6 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@81 PS2, Line 81: st scan > Done totally nitpicking: I disagree in this case, because lock() can suggests to someone that the function actually locks something, while GetLock() is really unambiguous. http://gerrit.cloudera.org:8080/#/c/11444/5/be/src/runtime/io/hdfs-file-reader.cc File be/src/runtime/io/hdfs-file-reader.cc: http://gerrit.cloudera.org:8080/#/c/11444/5/be/src/runtime/io/hdfs-file-reader.cc@118 PS5, Line 118: DCHECK_GE(chunk_size, 0); I think that this should be DCHECK_GT - (bytes_to_read - *bytes_read) is positive because of the loop condition and max_chunk_size should be also positive. http://gerrit.cloudera.org:8080/#/c/11444/5/be/src/runtime/io/hdfs-file-reader.cc@131 PS5, Line 131: // Do not retry: : // - if read was successful (current_bytes_read != -1) : // - or if already retried once : // - or if this not using a borrowed file handle : DCHECK_LE(num_retries, 1); : if (current_bytes_read != -1 || borrowed_hdfs_fh == nullptr : || num_retries == 1) { : break; : } Now that the "logic that is retried" is much simpler I would consider to rewrite this "while(true)" loop to an "if" or "for": status = ReadFromPosInternal(...) if (!status.ok() && borrowed_hdfs_fh == nullptr) { ... retry logic } http://gerrit.cloudera.org:8080/#/c/11444/5/be/src/runtime/io/hdfs-file-reader.cc@150 PS5, Line 150: if (!status.ok()) A DCHECK could be added to ensure that current_bytes_read == -1 iff !status.ok(). This would make it more obvious that the rest of the loop does not have to deal wirh negative current_bytes_read. http://gerrit.cloudera.org:8080/#/c/11444/5/be/src/runtime/io/hdfs-file-reader.cc@190 PS5, Line 190: seek_failed This variable is no longer used. http://gerrit.cloudera.org:8080/#/c/11444/4/be/src/runtime/io/local-file-reader.cc File be/src/runtime/io/local-file-reader.cc: http://gerrit.cloudera.org:8080/#/c/11444/4/be/src/runtime/io/local-file-reader.cc@55 PS4, Line 55: unique_lock<SpinLock> fs_lock(lock_); : DCHECK(scan_range_->read_in_flight()); : DCHECK_GE(bytes_to_read, 0); : // Delay before acquiring the lock, to allow triggering IMPALA-6587 race. : #ifndef NDEBUG : if (FLAGS_stress_disk_read_delay_ms > 0) { : SleepForMs(FLAGS_stress_disk_read_delay_ms); : } : #endif The comment contradicts the code because the lock is acquired before the sleep. The original function and the HdfsFileReader version acquires the lock after the sleep. It may also make sense to move this logic to a common function in FileReader that would call to something like protected ReadFromPosImpl(). -- 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: 5 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 13:38:50 +0000 Gerrit-HasComments: Yes
