Marcel Kornacker has posted comments on this change. Change subject: IMPALA-4996: Single-threaded KuduScanNode ......................................................................
Patch Set 1: (12 comments) http://gerrit.cloudera.org:8080/#/c/6312/1//COMMIT_MSG Commit Message: Line 11: same condition as HdfsScanNodeMt: mt_dop is greater than 1. >= 1? http://gerrit.cloudera.org:8080/#/c/6312/1/be/src/exec/kudu-scan-node-base.cc File be/src/exec/kudu-scan-node-base.cc: Line 57: : ScanNode(pool, tnode, descs), is this standard indentation? http://gerrit.cloudera.org:8080/#/c/6312/1/be/src/exec/kudu-scan-node-base.h File be/src/exec/kudu-scan-node-base.h: Line 79: /// The Kudu client and table. Scanners share these instances. how much overhead does not sharing this imply? (how much memory/how many threads sit behind these structures?) if it's a lot, we could share this across fragment instances in the query state. Line 90: /// The next index in 'scan_tokens_' to be assigned. Protected by lock_. there is no lock_ here Line 93: RuntimeProfile::Counter* kudu_round_trips_; these are still in KuduScanNode Line 102: RuntimeProfile::Counter* kudu_round_trips() const { return kudu_round_trips_; } why is this useful? since it's private, it's only accessible to this class anyway. http://gerrit.cloudera.org:8080/#/c/6312/1/be/src/exec/kudu-scan-node-mt.cc File be/src/exec/kudu-scan-node-mt.cc: Line 60: if (scanner_ == nullptr) { why not do this in open? Line 81: scanner_.reset(); why reset here? http://gerrit.cloudera.org:8080/#/c/6312/1/be/src/exec/kudu-scan-node-mt.h File be/src/exec/kudu-scan-node-mt.h: Line 45: std::unique_ptr<KuduScanner> scanner_; the separation of scannode and scanner exists today because of the multi-threading in the scannode. for the mt version, that's not needed anymore. let's either merge the scanner into this class now, or leave a todo to do that when we get rid of the non-mt version. the advantage is that you remove a level of indirection and get a chance to remove other overhead (example: right now, you create a copy of the conjunct contexts) http://gerrit.cloudera.org:8080/#/c/6312/1/fe/src/main/java/org/apache/impala/planner/KuduScanNode.java File fe/src/main/java/org/apache/impala/planner/KuduScanNode.java: Line 141: // Determine backend scan node implementation to use. The optimized MT implementation fix comment http://gerrit.cloudera.org:8080/#/c/6312/1/tests/query_test/test_mt_dop.py File tests/query_test/test_mt_dop.py: Line 102: # Parquet filtering test rlies on a specific mt_dop value, so keep in its own test spelling Line 118: # Parquet filtering test rlies on a specific mt_dop value, so keep in its own test spelling -- To view, visit http://gerrit.cloudera.org:8080/6312 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I6e4593300e376bc508b78acaea64ffdd2c73a67a Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Joe McDonnell <[email protected]> Gerrit-Reviewer: Marcel Kornacker <[email protected]> Gerrit-HasComments: Yes
