Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/15096 )
Change subject: WIP: IMPALA-9156: share broadcast join builds ...................................................................... Patch Set 7: (6 comments) Addressed comments and fixed my TODO about estimating # of instances. I need to add some more tests, but the code is complete to the best of my knowledge http://gerrit.cloudera.org:8080/#/c/15096/7/fe/src/main/java/org/apache/impala/planner/HBaseScanNode.java File fe/src/main/java/org/apache/impala/planner/HBaseScanNode.java: http://gerrit.cloudera.org:8080/#/c/15096/7/fe/src/main/java/org/apache/impala/planner/HBaseScanNode.java@362 PS7, Line 362: // Assume that each executor in the cluster gets a scan range, unless there are fewer : // scan ranges than nodes. > Could be updated to mention instances. Done http://gerrit.cloudera.org:8080/#/c/15096/7/fe/src/main/java/org/apache/impala/planner/HBaseScanNode.java@371 PS7, Line 371: LOG.trace("computeStats HbaseScan: #nodes=" + numNodes_); > numInstances could be also added to the log Done http://gerrit.cloudera.org:8080/#/c/15096/7/fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java File fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java: http://gerrit.cloudera.org:8080/#/c/15096/7/fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java@1243 PS7, Line 1243: g > nit: could reuse maxInstancesPerNode Done http://gerrit.cloudera.org:8080/#/c/15096/7/fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java@1269 PS7, Line 1269: dummyHostIndex.size() > using totalNodes would work the same bit seems a bit clearer. Done http://gerrit.cloudera.org:8080/#/c/15096/7/fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java@1271 PS7, Line 1271: + "TotalNodes %d, Local Ranges %d", > Maybe add totalInstances? Done http://gerrit.cloudera.org:8080/#/c/15096/7/fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java@1313 PS7, Line 1313: // Exit early if we have maxed out our estimate of hosts/instances, to avoid : // extraneous work in case the number of scan ranges dominates the number of : // nodes. > This is out of scope, but this whole loop seems more complicated to me than Yeah I agree, it does seem overly complex for what it's trying to do. -- To view, visit http://gerrit.cloudera.org:8080/15096 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I4c67e4b2c87ed0fba648f1e1710addb885d66dc7 Gerrit-Change-Number: 15096 Gerrit-PatchSet: 7 Gerrit-Owner: Tim Armstrong <[email protected]> Gerrit-Reviewer: Bikramjeet Vig <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Comment-Date: Wed, 26 Feb 2020 21:17:51 +0000 Gerrit-HasComments: Yes
