Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/10810 )
Change subject: IMPALA-7095: clean up scan node profiles ...................................................................... Patch Set 7: (14 comments) http://gerrit.cloudera.org:8080/#/c/10810/7/be/src/exec/data-source-scan-node.cc File be/src/exec/data-source-scan-node.cc: http://gerrit.cloudera.org:8080/#/c/10810/7/be/src/exec/data-source-scan-node.cc@359 PS7, Line 359: COUNTER_ADD(rows_read_counter_, rows_read); > Any idea why rows_returned_counter_ is updated at line 355 but rows_read_co COUNTER_SET is idempotent and is safe to call more times. Agree its ok to move it into this conditional. http://gerrit.cloudera.org:8080/#/c/10810/7/be/src/exec/hbase-scan-node.h File be/src/exec/hbase-scan-node.h: http://gerrit.cloudera.org:8080/#/c/10810/7/be/src/exec/hbase-scan-node.h@119 PS7, Line 119: time > wall clock time Done http://gerrit.cloudera.org:8080/#/c/10810/7/be/src/exec/hdfs-scan-node-base.h File be/src/exec/hdfs-scan-node-base.h: http://gerrit.cloudera.org:8080/#/c/10810/7/be/src/exec/hdfs-scan-node-base.h@126 PS7, Line 126: throughput > nit: adding (bytes/sec) like below in the definition of the field seems he Done http://gerrit.cloudera.org:8080/#/c/10810/7/be/src/exec/hdfs-scan-node-base.h@150 PS7, Line 150: WARN_UNUSED_RESULT; > nit: long line. Done http://gerrit.cloudera.org:8080/#/c/10810/7/be/src/exec/hdfs-scan-node-base.h@521 PS7, Line 521: // > nit: /// Done http://gerrit.cloudera.org:8080/#/c/10810/7/be/src/exec/kudu-scan-node-base.h File be/src/exec/kudu-scan-node-base.h: http://gerrit.cloudera.org:8080/#/c/10810/7/be/src/exec/kudu-scan-node-base.h@41 PS7, Line 41: : virtual Status Prepare(RuntimeState* state) override; : virtual Status Open(RuntimeState* state) override; : virtual Status GetNext(RuntimeState* state, RowBatch* row_batch, bool* eos) : override = 0; : protected: : virtual void DebugString(int indentation_level, std::stringstream* out) const override; > WARN_UNUSED_RESULT; It's not really necessary now that the clang precommit builds detect dropped statuses. Not sure if it actually has an effect even on GCC since we'll be generally calling these polymorphically via an ExecNode* http://gerrit.cloudera.org:8080/#/c/10810/7/be/src/exec/kudu-scan-node.h File be/src/exec/kudu-scan-node.h: http://gerrit.cloudera.org:8080/#/c/10810/7/be/src/exec/kudu-scan-node.h@44 PS7, Line 44: virtual Status Prepare(RuntimeState* state) override; > WARN_UNUSED_RESULT; Same below . See comment on KuduScanNodeBase http://gerrit.cloudera.org:8080/#/c/10810/7/be/src/exec/scan-node.h File be/src/exec/scan-node.h: http://gerrit.cloudera.org:8080/#/c/10810/7/be/src/exec/scan-node.h@87 PS7, Line 87: the fragment execution thread > scanner threads Done http://gerrit.cloudera.org:8080/#/c/10810/7/be/src/exec/scan-node.h@202 PS7, Line 202: used by > only used by Done http://gerrit.cloudera.org:8080/#/c/10810/7/be/src/exec/scan-node.h@209 PS7, Line 209: class ScannerThreadState { > I suppose this class is mostly thread-safe as most fields are backed by ato Good point. I updated comments to clarify which methods were thread-safe. http://gerrit.cloudera.org:8080/#/c/10810/7/be/src/exec/scan-node.h@211 PS7, Line 211: state > Mind elaborating ? This is mostly for initializing the counters, right ? Done http://gerrit.cloudera.org:8080/#/c/10810/7/be/src/exec/scan-node.h@214 PS7, Line 214: state > Open() creates the row batch queue, right ? Done http://gerrit.cloudera.org:8080/#/c/10810/7/be/src/exec/scan-node.h@255 PS7, Line 255: private: > nit: blank line before private makes it more legible. Done http://gerrit.cloudera.org:8080/#/c/10810/7/be/src/exec/scan-node.h@271 PS7, Line 271: boost::scoped_ptr< > std::unique_ptr<> Aren't we moving away from boost in the long run ? I think that's an open question. Using scoped_ptr implies that the pointer isn't movable so provides some additional information. We document this pattern here: https://cwiki.apache.org/confluence/display/IMPALA/Resource+Management+Best+Practices+in+Impala That argument makes sense to me but I don't feel super-strongly. Are we moving away from it in other areas of the codebase? I might start a dev@ thread to see how people feel. -- To view, visit http://gerrit.cloudera.org:8080/10810 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I77286282d42e7764bfdf94c7ec47cec9d743f787 Gerrit-Change-Number: 10810 Gerrit-PatchSet: 7 Gerrit-Owner: Tim Armstrong <[email protected]> Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Reviewer: Zoltan Borok-Nagy <[email protected]> Gerrit-Comment-Date: Thu, 05 Jul 2018 23:34:22 +0000 Gerrit-HasComments: Yes
