impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Marcel Kornacker (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-4996: Single-threaded KuduScanNode
Date Wed, 08 Mar 2017 20:49:53 GMT
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 <joemcdonnell@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <marcel@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message