Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/14357 )
Change subject: IMPALA-8998: admission control accounting for mt_dop ...................................................................... Patch Set 8: (7 comments) I won't carry +1 since the startup flag change was kinda significant. http://gerrit.cloudera.org:8080/#/c/14357/8/be/src/runtime/exec-env.h File be/src/runtime/exec-env.h: http://gerrit.cloudera.org:8080/#/c/14357/8/be/src/runtime/exec-env.h@267 PS8, Line 267: /// The maximum number of admission slots that should be used on this host. > Maybe explain (here or elsewhere) what "slot" means, e.g. that it's concept I added a bit more of an explanation here. I don't want to get too in depth in this comment, but agree some more context is useful. Tried to make it roughly consistent with admit_mem_limit as far as the detail level. http://gerrit.cloudera.org:8080/#/c/14357/8/be/src/runtime/exec-env.cc File be/src/runtime/exec-env.cc: http://gerrit.cloudera.org:8080/#/c/14357/8/be/src/runtime/exec-env.cc@90 PS8, Line 90: "(Advanced) The maximum degree of parallelism to run queries with on this backend. " > Maybe we should create a new flag with a better name and the same behavior, That's a good point. I just added a new flag that takes precedence over the old one. We can probably leave the old one around indefinitely TBH, the code to do the fallback isn't complex. http://gerrit.cloudera.org:8080/#/c/14357/8/be/src/scheduling/admission-controller.h File be/src/scheduling/admission-controller.h: http://gerrit.cloudera.org:8080/#/c/14357/8/be/src/scheduling/admission-controller.h@252 PS8, Line 252: > nit: double space Done http://gerrit.cloudera.org:8080/#/c/14357/8/be/src/scheduling/admission-controller.cc File be/src/scheduling/admission-controller.cc: http://gerrit.cloudera.org:8080/#/c/14357/8/be/src/scheduling/admission-controller.cc@563 PS8, Line 563: HasAvailableSlot > nit: rename to HasAvailableSlots()? Done http://gerrit.cloudera.org:8080/#/c/14357/8/testdata/workloads/functional-query/queries/QueryTest/mt-dop-parquet-admission-slots.test File testdata/workloads/functional-query/queries/QueryTest/mt-dop-parquet-admission-slots.test: http://gerrit.cloudera.org:8080/#/c/14357/8/testdata/workloads/functional-query/queries/QueryTest/mt-dop-parquet-admission-slots.test@3 PS8, Line 3: 4 > This is because we set mt_dop=4 and 24/3 > 4, right? It might be good to me Done http://gerrit.cloudera.org:8080/#/c/14357/8/tests/custom_cluster/test_executor_groups.py File tests/custom_cluster/test_executor_groups.py: http://gerrit.cloudera.org:8080/#/c/14357/8/tests/custom_cluster/test_executor_groups.py@218 PS8, Line 218: gets > nit: get Done http://gerrit.cloudera.org:8080/#/c/14357/8/tests/verifiers/metric_verifier.py File tests/verifiers/metric_verifier.py: http://gerrit.cloudera.org:8080/#/c/14357/8/tests/verifiers/metric_verifier.py@69 PS8, Line 69: wait_for_backends_state > maybe this method could have a more descriptive name? Done -- To view, visit http://gerrit.cloudera.org:8080/14357 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I7b6b6262ef238df26b491352656a26e4163e46e5 Gerrit-Change-Number: 14357 Gerrit-PatchSet: 8 Gerrit-Owner: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Reviewer: Abhishek Rawat <ara...@cloudera.com> Gerrit-Reviewer: Andrew Sherman <asher...@cloudera.com> Gerrit-Reviewer: Bikramjeet Vig <bikramjeet....@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Lars Volker <l...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Comment-Date: Thu, 03 Oct 2019 23:07:17 +0000 Gerrit-HasComments: Yes
