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 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 > totally nitpicking: I disagree in this case, because lock() can suggests to For now I leave it as it is but I see your point. 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 po Done 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 re Done 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 Done 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. Done 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 sl Nice catch! Yeah, I first wanted to use the template method pattern, i.e. have non-virtual public member functions that call virtual private functions that are overridden by the children. However, there's little common code so I'm not sure if it worth the trouble. For now I leave it as it is. -- 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 14:26:36 +0000 Gerrit-HasComments: Yes
