Joe McDonnell has posted comments on this change. Change subject: IMPALA-4996: Single-threaded KuduScanNode ......................................................................
Patch Set 2: (19 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? Done http://gerrit.cloudera.org:8080/#/c/6312/2//COMMIT_MSG Commit Message: Line 1: Parent: def5355a (IMPALA-3403: [DOCS] Pare back irrelevant installation info) > nit: can you wrap lines in git commits at 60 chars? formatting in git tools Done 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? Yes, this is what is listed in the Google C++ style guide. http://gerrit.cloudera.org:8080/#/c/6312/2/be/src/exec/kudu-scan-node-base.cc File be/src/exec/kudu-scan-node-base.cc: PS2, Line 122: Base > remove Base for the debugging output Done 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 35: /// are used to retrieve the rows for this scan. > You should make this comment more specific to KuduScanNodeBase, eg. "Base c Improved the comment. Line 79: /// The Kudu client and table. Scanners share these instances. > how much overhead does not sharing this imply? (how much memory/how many th Moved the KuduClient to the QueryState. Line 90: /// The next index in 'scan_tokens_' to be assigned. Protected by lock_. > there is no lock_ here Done Line 93: RuntimeProfile::Counter* kudu_round_trips_; > these are still in KuduScanNode Done PS1, Line 98: // > /// Done 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 KuduScanner is a friend class, so it accesses these elements. In this upload, I have KuduScanner avoid direct field accesses, but I don't have a strong preference. This could be removed (along with tuple_desc() and kudu_client()) and KuduScanner could access the fields directly. 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? Moved to open. Line 81: scanner_.reset(); > why reset here? Now that I think about it, I don't think it is needed. If the caller gets an error, it will call Close. 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-th Added a comment. http://gerrit.cloudera.org:8080/#/c/6312/2/be/src/exec/kudu-scan-node.h File be/src/exec/kudu-scan-node.h: PS2, Line 79: : RuntimeProfile::Counter* kudu_round_trips_; : RuntimeProfile::Counter* kudu_remote_tokens_; : static const std::string KUDU_ROUND_TRIPS; : static const std::string KUDU_REMOTE_TOKENS; > remove; aren't these in the base class? Done 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 Done 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 This will go away when I rebase. Line 118: # Parquet filtering test rlies on a specific mt_dop value, so keep in its own test > spelling Removed comment. It was wrong anyway. http://gerrit.cloudera.org:8080/#/c/6312/2/tests/query_test/test_mt_dop.py File tests/query_test/test_mt_dop.py: PS2, Line 102: rlies > typo This will be gone when I rebase. PS2, Line 118: # Parquet filtering test rlies on a specific mt_dop value, so keep in its own test > update comment Done -- 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: 2 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Joe McDonnell <[email protected]> Gerrit-Reviewer: Joe McDonnell <[email protected]> Gerrit-Reviewer: Marcel Kornacker <[email protected]> Gerrit-Reviewer: Matthew Jacobs <[email protected]> Gerrit-Reviewer: Thomas Tauber-Marshall <[email protected]> Gerrit-HasComments: Yes
