Joe McDonnell has posted comments on this change. ( http://gerrit.cloudera.org:8080/17062 )
Change subject: IMPALA-10504: Add tracing for remote block reads ...................................................................... Patch Set 3: (5 comments) http://gerrit.cloudera.org:8080/#/c/17062/3//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/17062/3//COMMIT_MSG@9 PS3, Line 9: first unexpected remote read of each : scanrange Does this happen for large numbers of scan ranges simultaneously or is it more sporadic? I'm just trying to get a sense of how much output this could produce. http://gerrit.cloudera.org:8080/#/c/17062/3//COMMIT_MSG@13 PS3, Line 13: Reviewed-by: Aman Sinha <amsi...@cloudera.com> Nit: Remove this line (gerrit will add these lines when this merges). http://gerrit.cloudera.org:8080/#/c/17062/3/be/src/runtime/io/hdfs-file-reader.cc File be/src/runtime/io/hdfs-file-reader.cc: http://gerrit.cloudera.org:8080/#/c/17062/3/be/src/runtime/io/hdfs-file-reader.cc@81 PS3, Line 81: appendHostList(const hdfsFS hdfs_fs, const char* file_name, : int64_t file_offset, int64_t bytes_to_read, ostream& ostr) { Nit: Couple style things It may make sense to make this a private method on HdfsFileReader rather than a regular C-style function. The hdfs_fs and file_name both come from the class anyway. Either way, it would be nice to have a comment describing the expected behavior. For this, I think it would also be cleaner not to have to pass in the ostream. One way to do it would be to use a std::ostringstream to collect the individual logging statements and then have this function return a std::string (i.e. ostreamstream's str() method). Then the logging would look like: LOG(INFO) << "Normal log stuff" << appendHostList(fs, filename, offset, ...); http://gerrit.cloudera.org:8080/#/c/17062/3/be/src/runtime/io/hdfs-file-reader.cc@232 PS3, Line 232: int64_t save_num_remote_bytes = num_remote_bytes_; Nit: Since we are using this like a boolean, does it make sense to have a boolean saying whether we have printed the unexpected remote logging already? http://gerrit.cloudera.org:8080/#/c/17062/3/be/src/runtime/io/hdfs-file-reader.cc@242 PS3, Line 242: num_remote_bytes_-save_num_remote_bytes Nit: We only enter this if save_num_remote_bytes==0, so we could drop this. -- To view, visit http://gerrit.cloudera.org:8080/17062 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I8c6a3e92f44813048022edf2b91299b3b0a20257 Gerrit-Change-Number: 17062 Gerrit-PatchSet: 3 Gerrit-Owner: Kurt Deschler <kdesc...@cloudera.com> Gerrit-Reviewer: Aman Sinha <amsi...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Joe McDonnell <joemcdonn...@cloudera.com> Gerrit-Comment-Date: Wed, 17 Feb 2021 01:38:20 +0000 Gerrit-HasComments: Yes
