Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/18143 )
Change subject: IMPALA-10992 Planner changes for estimate peak memory - v1 ...................................................................... Patch Set 29: (20 comments) http://gerrit.cloudera.org:8080/#/c/18143/29//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/18143/29//COMMIT_MSG@9 PS29, Line 9: a set of executor : groups nit: multiple executor group sets. Change mentions of executor group to executor group sets where applicable in the commit message. This is to stay consistent with the distinction between executor groups vs group sets http://gerrit.cloudera.org:8080/#/c/18143/29//COMMIT_MSG@25 PS29, Line 25: To facilitate testing, the patch imposes an artificial two executor : group setup in FE. This setup is enabled when 'enable_replan' is set : to 3 (testing) or RuntimeEnv.INSTANCE.isTestEnv() is true as in most : frontend tests. The artificial two executor groups are configured as : follows. Does this mean all tests are run in this configuration? All planner tests and end2end pytest are currently based on a minicluster or a custom cluster to emulate an actual cluster. Please see my comment in Frontend.java about how we can ensure re-planning code-path gets tested while ensuring the actual group set gets used. http://gerrit.cloudera.org:8080/#/c/18143/29/common/thrift/Frontend.thrift File common/thrift/Frontend.thrift: http://gerrit.cloudera.org:8080/#/c/18143/29/common/thrift/Frontend.thrift@729 PS29, Line 729: The optional threshold for queries to run in an executor group nit: can you provide more context here as to what this threshold signifies, like which resource is represents and how it helps the planner make decisions. http://gerrit.cloudera.org:8080/#/c/18143/29/common/thrift/Query.thrift File common/thrift/Query.thrift: http://gerrit.cloudera.org:8080/#/c/18143/29/common/thrift/Query.thrift@585 PS29, Line 585: TEnableReplanMode.TESTING shouldn't this be set to ON by default as per the comment? http://gerrit.cloudera.org:8080/#/c/18143/29/fe/src/main/java/org/apache/impala/service/Frontend.java File fe/src/main/java/org/apache/impala/service/Frontend.java: http://gerrit.cloudera.org:8080/#/c/18143/29/fe/src/main/java/org/apache/impala/service/Frontend.java@267 PS29, Line 267: /////////////////////////////////////////// : // BEGIN: Data members used by auto-scaling nit: would it make sense to put this inside a separate State class? http://gerrit.cloudera.org:8080/#/c/18143/29/fe/src/main/java/org/apache/impala/service/Frontend.java@286 PS29, Line 286: // The stmt table cache to be used by all iterations of auto-scaling compilation. nit: mention when it is set and when can it be reset http://gerrit.cloudera.org:8080/#/c/18143/29/fe/src/main/java/org/apache/impala/service/Frontend.java@339 PS29, Line 339: auto-scaling nit: not sure auto-scaling is the right term here, since we are not scaling anything but rather assigning a query to the right group. Open to suggestions for more semantically appropriate terms. http://gerrit.cloudera.org:8080/#/c/18143/29/fe/src/main/java/org/apache/impala/service/Frontend.java@1763 PS29, Line 1763: setupThresholdsForGroupSets nit: add a comment about what it does http://gerrit.cloudera.org:8080/#/c/18143/29/fe/src/main/java/org/apache/impala/service/Frontend.java@1780 PS29, Line 1780: enable_replan == TEnableReplanMode.TESTING nit: also might be worth logging this while planning http://gerrit.cloudera.org:8080/#/c/18143/29/fe/src/main/java/org/apache/impala/service/Frontend.java@1787 PS29, Line 1787: TExecutorGroupSet l = new TExecutorGroupSet(3, 3, "large"); : l.setThreshold(Long.MAX_VALUE); : result.add(l); if we want to emulate a 2 exec group set configuration, what if we only add a dummy executor group set which will always be rejected (i think its the 'regular' one in this case) and the other one as the actual group set 'e = executorGroupSets.get(0)'. Otherwise this can get very confusing if the someone is running a test (and is not aware of this) on a different configuration but the plan always gets created for a 3 node group set. http://gerrit.cloudera.org:8080/#/c/18143/29/fe/src/main/java/org/apache/impala/service/Frontend.java@1792 PS29, Line 1792: executorGroupSets.get(0) nit: can just use 'e' here too. http://gerrit.cloudera.org:8080/#/c/18143/29/fe/src/main/java/org/apache/impala/service/Frontend.java@1801 PS29, Line 1801: request_pool if we use query_exec_request.query_ctx.request_pool instead of queryOptions.getRequest_pool(), that should have the resolved request pool that always has the "root." prefix http://gerrit.cloudera.org:8080/#/c/18143/29/fe/src/main/java/org/apache/impala/service/Frontend.java@1802 PS29, Line 1802: if (request_pool != null && !e.getExec_group_name_prefix().endsWith(request_pool)) { : continue; : } what happens if a user specifies a request pool that does not match to any? http://gerrit.cloudera.org:8080/#/c/18143/29/fe/src/main/java/org/apache/impala/service/Frontend.java@1924 PS29, Line 1924: // Find out the per host memory estimated from two possible sources. : per_host_mem_estimate = -1; : if (req.query_exec_request != null) { : per_host_mem_estimate = req.query_exec_request.per_host_mem_estimate; : } else { : per_host_mem_estimate = planCtx.getEstimatedMemoryPerHost(); : } when would either case be used? as in, why would query_exec_request not be set? for explain plans? http://gerrit.cloudera.org:8080/#/c/18143/29/fe/src/main/java/org/apache/impala/service/Frontend.java@1933 PS29, Line 1933: per_host_mem_estimate == -1 when can this happen? http://gerrit.cloudera.org:8080/#/c/18143/29/fe/src/main/java/org/apache/impala/service/Frontend.java@1947 PS29, Line 1947: LOG.info("Selected executor group: " + group_set + ", reason: " + reason); nit: should we add this to the query profile? but only if default_executor_group is false http://gerrit.cloudera.org:8080/#/c/18143/29/fe/src/main/java/org/apache/impala/util/ClassUtil.java File fe/src/main/java/org/apache/impala/util/ClassUtil.java: http://gerrit.cloudera.org:8080/#/c/18143/29/fe/src/main/java/org/apache/impala/util/ClassUtil.java@31 PS29, Line 31: getStackTraceForThread is this used anywhere? http://gerrit.cloudera.org:8080/#/c/18143/29/fe/src/main/java/org/apache/impala/util/ExecutorMembershipSnapshot.java File fe/src/main/java/org/apache/impala/util/ExecutorMembershipSnapshot.java: http://gerrit.cloudera.org:8080/#/c/18143/29/fe/src/main/java/org/apache/impala/util/ExecutorMembershipSnapshot.java@74 PS29, Line 74: currentExecutorGroupSet_ as we discussed earlier, we need to save this state for each plan call as ExecutorMembershipSnapshot is a singleton http://gerrit.cloudera.org:8080/#/c/18143/29/testdata/workloads/functional-planner/queries/PlannerTest/resource-requirements.test File testdata/workloads/functional-planner/queries/PlannerTest/resource-requirements.test: http://gerrit.cloudera.org:8080/#/c/18143/29/testdata/workloads/functional-planner/queries/PlannerTest/resource-requirements.test@1912 PS29, Line 1912: 287MB why did these estimates change? http://gerrit.cloudera.org:8080/#/c/18143/29/tests/custom_cluster/test_executor_groups.py File tests/custom_cluster/test_executor_groups.py: http://gerrit.cloudera.org:8080/#/c/18143/29/tests/custom_cluster/test_executor_groups.py@608 PS29, Line 608: test_admission_control_with_multiple_coords_and_exec_groups can you write a similar test where the right set of executor groups sets are started and a reasonable threshold is set via admission configs. Then it tests out 2 queries where one gets assigned to the first and the other gets assigned to the other. This way we can test the functionality in action end 2 end. -- To view, visit http://gerrit.cloudera.org:8080/18143 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ibe71f905d6a8c1e42cf951b3a69ff33b81277c24 Gerrit-Change-Number: 18143 Gerrit-PatchSet: 29 Gerrit-Owner: Qifan Chen <[email protected]> Gerrit-Reviewer: Aman Sinha <[email protected]> Gerrit-Reviewer: Bikramjeet Vig <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Kurt Deschler <[email protected]> Gerrit-Reviewer: Qifan Chen <[email protected]> Gerrit-Reviewer: Wenzhe Zhou <[email protected]> Gerrit-Comment-Date: Fri, 25 Feb 2022 03:19:36 +0000 Gerrit-HasComments: Yes
