Riza Suminto has posted comments on this change. ( http://gerrit.cloudera.org:8080/19807 )
Change subject: IMPALA-12091: Control scan parallelism by its processing cost ...................................................................... Patch Set 4: (17 comments) http://gerrit.cloudera.org:8080/#/c/19807/3/be/src/exec/exec-node.h File be/src/exec/exec-node.h: http://gerrit.cloudera.org:8080/#/c/19807/3/be/src/exec/exec-node.h@132 PS3, Line 132: int num_instances_per_node() const { return num_instances_per_node_; } > Would rather not see the term "effective" applied during execution. At this Renamed to 'num_instances_per_node' http://gerrit.cloudera.org:8080/#/c/19807/3/be/src/exec/hdfs-scanner.h File be/src/exec/hdfs-scanner.h: http://gerrit.cloudera.org:8080/#/c/19807/3/be/src/exec/hdfs-scanner.h@388 PS3, Line 388: PlanNode::num > nit: effective_dop() Replaced with PlanNode::num_instances_per_node() http://gerrit.cloudera.org:8080/#/c/19807/3/be/src/exec/scan-node.cc File be/src/exec/scan-node.cc: http://gerrit.cloudera.org:8080/#/c/19807/3/be/src/exec/scan-node.cc@298 PS3, Line 298: max_row_batches = > I think the scanner threads should be computed last, based on the planned p The more I look at this code, the more concern I am to change the existing formula. NUM_SCANNER_THREADS option have an impact on legacy parallelism mode, both MD_DOP=0 and MT_DOP>0. Changing them can lead to regression for user that is still using legacy parallelism mode. I prefer to not change them now, but rather override them into fixed count if COMPUTE_PROCESSING_COST=true, regardless of CpuInfo::num_cores() or NUM_SCANNER_THREADS value. I think this makes frontend planner decision more deterministic. http://gerrit.cloudera.org:8080/#/c/19807/3/be/src/scheduling/scheduler.cc File be/src/scheduling/scheduler.cc: http://gerrit.cloudera.org:8080/#/c/19807/3/be/src/scheduling/scheduler.cc@58 PS3, Line 58: > Please add comment and consider making this configurable. Added MAX_FRAGMENT_INSTANCES_PER_NODE constant in Query.thrift http://gerrit.cloudera.org:8080/#/c/19807/3/be/src/scheduling/scheduler.cc@275 PS3, Line 275: if (effective_instance_count < fragment_state->instance_states.size()) { > Let's address this case in this PR Done http://gerrit.cloudera.org:8080/#/c/19807/3/be/src/scheduling/scheduler.cc@283 PS3, Line 283: int largest_inst_per_host = 0; > Print values with message Done http://gerrit.cloudera.org:8080/#/c/19807/3/be/src/scheduling/scheduler.cc@289 PS3, Line 289: for (int i = 1; i < fragment_state->instance_states.size(); i++) { > largest_inst_idx Done http://gerrit.cloudera.org:8080/#/c/19807/3/be/src/scheduling/scheduler.cc@290 PS3, Line 290: if (fragment_state->instance_states[i].host > Add a comment to explain what the following blocks are checking. It's not c Done http://gerrit.cloudera.org:8080/#/c/19807/3/be/src/scheduling/scheduler.cc@313 PS3, Line 313: fragment_state->fragment.display_name, largest_inst_per_host, > Rename planned_inst_per_host I missed this, will rename it in next patchset. http://gerrit.cloudera.org:8080/#/c/19807/3/be/src/scheduling/scheduler.cc@322 PS3, Line 322: << " Consider running the query with COMPUTE_PROCESSING_COST=false." > Can we suggest anything to correct if this occurs? Add " Consider running the query with COMPUTE_PROCESSING_COST=false." to error message. All returned error status from this method were previously a DCHECK. I don't expect them to get raised, but if it does, I prefer this as returned status to client instead of DCHECK so I can debug it faster. http://gerrit.cloudera.org:8080/#/c/19807/3/be/src/service/query-options.h File be/src/service/query-options.h: http://gerrit.cloudera.org:8080/#/c/19807/3/be/src/service/query-options.h@293 PS3, Line 293: QUERY_OPT_FN(max_fragment_instances_per_node, MAX_FRAGMENT_INSTANCES_PER_NODE, \ > We should find a name that implies per-fragment per-instance. This is a lim Renamed to MAX_FRAGMENT_INSTANCES_PER_NODE. I'm leaving PROCESSING_COST_MIN_THREADS unchanged for now. http://gerrit.cloudera.org:8080/#/c/19807/3/common/thrift/ImpalaService.thrift File common/thrift/ImpalaService.thrift: http://gerrit.cloudera.org:8080/#/c/19807/3/common/thrift/ImpalaService.thrift@793 PS3, Line 793: MAX_FRAGMENT_INSTANCES_PER_NODE = 156 > Typo 156 Done http://gerrit.cloudera.org:8080/#/c/19807/3/common/thrift/Query.thrift File common/thrift/Query.thrift: http://gerrit.cloudera.org:8080/#/c/19807/3/common/thrift/Query.thrift@632 PS3, Line 632: // See comment in ImpalaService.thrift > Agree. The ranges of PROCESSING_COST_MAX_THREADS and PROCESSING_COST_MIN_TH Added MAX_FRAGMENT_INSTANCES_PER_NODE constant in Query.thrift http://gerrit.cloudera.org:8080/#/c/19807/3/fe/src/main/java/org/apache/impala/analysis/Analyzer.java File fe/src/main/java/org/apache/impala/analysis/Analyzer.java: http://gerrit.cloudera.org:8080/#/c/19807/3/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@165 PS3, Line 165: public final static String DB_ALREADY_EXISTS_ERROR_MSG = "Database already exists: "; > max_fragment_threads_per_instance Replaced with MAX_FRAGMENT_INSTANCES_PER_NODE constant in Query.thrift http://gerrit.cloudera.org:8080/#/c/19807/3/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@658 PS3, Line 658: > Returned value will be Math.max against getMinParallelismPerNode(). But I'l Done http://gerrit.cloudera.org:8080/#/c/19807/3/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@660 PS3, Line 660: Math.min(QueryConstants.MAX_FRAGMENT_INSTANCES_PER_NODE, x); > Is 0 expected here and handled later? Replaced with 1. http://gerrit.cloudera.org:8080/#/c/19807/3/fe/src/main/java/org/apache/impala/planner/ScanNode.java File fe/src/main/java/org/apache/impala/planner/ScanNode.java: http://gerrit.cloudera.org:8080/#/c/19807/3/fe/src/main/java/org/apache/impala/planner/ScanNode.java@56 PS3, Line 56: // scan ranges than would have been estimated assuming a uniform distribution. > Unused? Done -- To view, visit http://gerrit.cloudera.org:8080/19807 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: If948e45455275d9a61a6cd5d6a30a8b98a7c729a Gerrit-Change-Number: 19807 Gerrit-PatchSet: 4 Gerrit-Owner: Riza Suminto <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Kurt Deschler <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Reviewer: Riza Suminto <[email protected]> Gerrit-Reviewer: Wenzhe Zhou <[email protected]> Gerrit-Comment-Date: Fri, 05 May 2023 20:36:59 +0000 Gerrit-HasComments: Yes
