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 6: (8 comments) 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@150 PS5, Line 150: break; > Done Actually the other combination (when status is ok but current_bytes_read is -1) is the one I wanted to check. http://gerrit.cloudera.org:8080/#/c/11444/6/be/src/runtime/io/hdfs-file-reader.cc File be/src/runtime/io/hdfs-file-reader.cc: http://gerrit.cloudera.org:8080/#/c/11444/6/be/src/runtime/io/hdfs-file-reader.cc@131 PS6, Line 131: // Retry if: : // - first read was not successful : // and : // - used a borrowed file handle I find this comment to be a bit redundant, but feel free to keep it if you disagree. http://gerrit.cloudera.org:8080/#/c/11444/6/be/src/runtime/io/local-file-reader.cc File be/src/runtime/io/local-file-reader.cc: http://gerrit.cloudera.org:8080/#/c/11444/6/be/src/runtime/io/local-file-reader.cc@22 PS6, Line 22: #include "runtime/io/request-context.h" Is this header really needed? http://gerrit.cloudera.org:8080/#/c/11444/6/be/src/runtime/io/local-file-reader.cc@24 PS6, Line 24: #include "util/hdfs-util.h" Is this header really needed? http://gerrit.cloudera.org:8080/#/c/11444/6/be/src/runtime/io/request-context.h File be/src/runtime/io/request-context.h: http://gerrit.cloudera.org:8080/#/c/11444/6/be/src/runtime/io/request-context.h@188 PS6, Line 188: LocalFileReader Does LocalFileReader actually use RequestContext's private members? http://gerrit.cloudera.org:8080/#/c/11444/6/be/src/runtime/io/request-context.h@189 PS6, Line 189: HdfsFileReader I would prefer to remove this friendship if it doesn't complicate code too much. My idea would be to move stats/counters pointers to a single public struct, and pass a pointer to this to HdfsFileReader. Such a struct would be meaningful in my opinion, because it would contain those members that are manipulated from more than one threads and do not need locking (as they are atomics or pointers to atomics). http://gerrit.cloudera.org:8080/#/c/11444/6/be/src/runtime/io/request-ranges.h File be/src/runtime/io/request-ranges.h: http://gerrit.cloudera.org:8080/#/c/11444/6/be/src/runtime/io/request-ranges.h@70 PS6, Line 70: LocalFileReader Is this friendship really needed? http://gerrit.cloudera.org:8080/#/c/11444/6/be/src/runtime/io/request-ranges.h@197 PS6, Line 197: LocalFileReader Is this friendship really needed? -- 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: 6 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 15:30:45 +0000 Gerrit-HasComments: Yes
