[Impala-ASF-CR] IMPALA-8536: Add Scalable Pool Configuration to Admission Controller.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13307 ) Change subject: IMPALA-8536: Add Scalable Pool Configuration to Admission Controller. .. Patch Set 16: Verified+1 -- To view, visit http://gerrit.cloudera.org:8080/13307 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: If47508728124076f3b9200c27cffc989f7a4f188 Gerrit-Change-Number: 13307 Gerrit-PatchSet: 16 Gerrit-Owner: Andrew Sherman Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Wed, 19 Jun 2019 05:58:08 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8536: Add Scalable Pool Configuration to Admission Controller.
Impala Public Jenkins has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/13307 ) Change subject: IMPALA-8536: Add Scalable Pool Configuration to Admission Controller. .. IMPALA-8536: Add Scalable Pool Configuration to Admission Controller. Add 3 configuration parameters to Admission Controller that scale with the number of hosts in the resource pool. These parameters are specified to the Impalad through the -llama_site_path flag which points to a Llama XML configuration file. The new configuration parameters are: + Max Running Queries Multiple - this floating point number is multiplied by the current total number of executors at runtime to give the maximum number of concurrently running queries allowed in the pool. This calculation is rounded up to the nearest integer so the result will always be at least one as long as the parameter is non-zero. + Max Queued Queries Multiple - this floating point number is multiplied by the current total number of executors at runtime to give the maximum number of queries that can be queued in the pool. This calculation is rounded up to the nearest integer so the result will always be at least one as long as the parameter is non-zero. + Max Memory Multiple - this number of bytes is multiplied by the current total number of executors at runtime to give the maximum memory available across the cluster for the pool. If any of these parameters have zero value then they will be ignored. In this case the corresponding non-scalable parameters will be used, if they are set. The new parameters are exposed through the webui. At various points in the code Admission Controller looks at the Pool Config objects to find non-scalable parameters such as the max number of queries that can run in the pool. These access have been encapsulated in functions that return the scalable version of the configuration value if the new scalable parameters are being used. Diagnostic messages are enhanced to show the origin of the encapsulated parameters. TESTING All end-to-end tests are running clean with ASAN. The unit test admission-controller-test.cc has been expanded to test the newly added code. Added an end-to-end test that adds and removes Impalads from a minicluster. Change-Id: If47508728124076f3b9200c27cffc989f7a4f188 Reviewed-on: http://gerrit.cloudera.org:8080/13307 Reviewed-by: Andrew Sherman Tested-by: Impala Public Jenkins --- M be/src/runtime/exec-env.cc M be/src/scheduling/admission-controller-test.cc M be/src/scheduling/admission-controller.cc M be/src/scheduling/admission-controller.h M common/thrift/ImpalaInternalService.thrift M common/thrift/generate_metrics.py M common/thrift/metrics.json M fe/src/main/java/org/apache/impala/util/RequestPoolService.java M fe/src/test/java/org/apache/impala/util/TestRequestPoolService.java M fe/src/test/resources/fair-scheduler-test.xml M fe/src/test/resources/fair-scheduler-test2.xml M fe/src/test/resources/llama-site-test.xml M fe/src/test/resources/llama-site-test2.xml M testdata/workloads/functional-query/queries/QueryTest/admission-max-min-mem-limits.test M testdata/workloads/functional-query/queries/QueryTest/admission-reject-mem-estimate.test M testdata/workloads/functional-query/queries/QueryTest/admission-reject-min-reservation.test M tests/common/impala_cluster.py M tests/custom_cluster/test_admission_controller.py M tests/webserver/test_web_pages.py M www/admission_controller.tmpl 20 files changed, 1,395 insertions(+), 302 deletions(-) Approvals: Andrew Sherman: Looks good to me, approved Impala Public Jenkins: Verified -- To view, visit http://gerrit.cloudera.org:8080/13307 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: If47508728124076f3b9200c27cffc989f7a4f188 Gerrit-Change-Number: 13307 Gerrit-PatchSet: 17 Gerrit-Owner: Andrew Sherman Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-8536: Add Scalable Pool Configuration to Admission Controller.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13307 ) Change subject: IMPALA-8536: Add Scalable Pool Configuration to Admission Controller. .. Patch Set 16: Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/4497/ DRY_RUN=false -- To view, visit http://gerrit.cloudera.org:8080/13307 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: If47508728124076f3b9200c27cffc989f7a4f188 Gerrit-Change-Number: 13307 Gerrit-PatchSet: 16 Gerrit-Owner: Andrew Sherman Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Wed, 19 Jun 2019 00:03:45 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8536: Add Scalable Pool Configuration to Admission Controller.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13307 ) Change subject: IMPALA-8536: Add Scalable Pool Configuration to Admission Controller. .. Patch Set 16: Verified-1 Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/4487/ -- To view, visit http://gerrit.cloudera.org:8080/13307 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: If47508728124076f3b9200c27cffc989f7a4f188 Gerrit-Change-Number: 13307 Gerrit-PatchSet: 16 Gerrit-Owner: Andrew Sherman Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Tue, 18 Jun 2019 21:27:42 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8536: Add Scalable Pool Configuration to Admission Controller.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13307 ) Change subject: IMPALA-8536: Add Scalable Pool Configuration to Admission Controller. .. Patch Set 15: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/3654/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/13307 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: If47508728124076f3b9200c27cffc989f7a4f188 Gerrit-Change-Number: 13307 Gerrit-PatchSet: 15 Gerrit-Owner: Andrew Sherman Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Tue, 18 Jun 2019 16:35:54 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8536: Add Scalable Pool Configuration to Admission Controller.
Andrew Sherman has posted comments on this change. ( http://gerrit.cloudera.org:8080/13307 ) Change subject: IMPALA-8536: Add Scalable Pool Configuration to Admission Controller. .. Patch Set 16: Code-Review+2 (4 comments) Bring forward Lars's +2 http://gerrit.cloudera.org:8080/#/c/13307/13/tests/custom_cluster/test_admission_controller.py File tests/custom_cluster/test_admission_controller.py: http://gerrit.cloudera.org:8080/#/c/13307/13/tests/custom_cluster/test_admission_controller.py@1045 PS13, Line 1045: ion > I see it bumped in L656 but not here. Given it has been used throughout the Done http://gerrit.cloudera.org:8080/#/c/13307/13/tests/custom_cluster/test_admission_controller.py@1045 PS13, Line 1045: ion > I just noticed that the parameter is seconds, so 100 should be perfectly fi OK I am moving these to 1000, I promise in future to think for myself and not believe what reviewers say :-) http://gerrit.cloudera.org:8080/#/c/13307/14/tests/custom_cluster/test_admission_controller.py File tests/custom_cluster/test_admission_controller.py: http://gerrit.cloudera.org:8080/#/c/13307/14/tests/custom_cluster/test_admission_controller.py@965 PS14, Line 965: mem_limit values that are pass > nit: remove one "passed"? Done http://gerrit.cloudera.org:8080/#/c/13307/13/tests/webserver/test_web_pages.py File tests/webserver/test_web_pages.py: http://gerrit.cloudera.org:8080/#/c/13307/13/tests/webserver/test_web_pages.py@508 PS13, Line 508: assert pool_config['max_memory_multiple'] == 0 > I think if they exist but at 0, these tests will fail. If the goal is only Thanks -- To view, visit http://gerrit.cloudera.org:8080/13307 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: If47508728124076f3b9200c27cffc989f7a4f188 Gerrit-Change-Number: 13307 Gerrit-PatchSet: 16 Gerrit-Owner: Andrew Sherman Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Tue, 18 Jun 2019 16:12:09 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8536: Add Scalable Pool Configuration to Admission Controller.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13307 ) Change subject: IMPALA-8536: Add Scalable Pool Configuration to Admission Controller. .. Patch Set 16: Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/4488/ DRY_RUN=false -- To view, visit http://gerrit.cloudera.org:8080/13307 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: If47508728124076f3b9200c27cffc989f7a4f188 Gerrit-Change-Number: 13307 Gerrit-PatchSet: 16 Gerrit-Owner: Andrew Sherman Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Tue, 18 Jun 2019 16:07:41 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8536: Add Scalable Pool Configuration to Admission Controller.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13307 ) Change subject: IMPALA-8536: Add Scalable Pool Configuration to Admission Controller. .. Patch Set 16: Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/4487/ DRY_RUN=false -- To view, visit http://gerrit.cloudera.org:8080/13307 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: If47508728124076f3b9200c27cffc989f7a4f188 Gerrit-Change-Number: 13307 Gerrit-PatchSet: 16 Gerrit-Owner: Andrew Sherman Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Tue, 18 Jun 2019 16:02:45 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8536: Add Scalable Pool Configuration to Admission Controller.
Andrew Sherman has uploaded a new patch set (#15). ( http://gerrit.cloudera.org:8080/13307 ) Change subject: IMPALA-8536: Add Scalable Pool Configuration to Admission Controller. .. IMPALA-8536: Add Scalable Pool Configuration to Admission Controller. Add 3 configuration parameters to Admission Controller that scale with the number of hosts in the resource pool. These parameters are specified to the Impalad through the -llama_site_path flag which points to a Llama XML configuration file. The new configuration parameters are: + Max Running Queries Multiple - this floating point number is multiplied by the current total number of executors at runtime to give the maximum number of concurrently running queries allowed in the pool. This calculation is rounded up to the nearest integer so the result will always be at least one as long as the parameter is non-zero. + Max Queued Queries Multiple - this floating point number is multiplied by the current total number of executors at runtime to give the maximum number of queries that can be queued in the pool. This calculation is rounded up to the nearest integer so the result will always be at least one as long as the parameter is non-zero. + Max Memory Multiple - this number of bytes is multiplied by the current total number of executors at runtime to give the maximum memory available across the cluster for the pool. If any of these parameters have zero value then they will be ignored. In this case the corresponding non-scalable parameters will be used, if they are set. The new parameters are exposed through the webui. At various points in the code Admission Controller looks at the Pool Config objects to find non-scalable parameters such as the max number of queries that can run in the pool. These access have been encapsulated in functions that return the scalable version of the configuration value if the new scalable parameters are being used. Diagnostic messages are enhanced to show the origin of the encapsulated parameters. TESTING All end-to-end tests are running clean with ASAN. The unit test admission-controller-test.cc has been expanded to test the newly added code. Added an end-to-end test that adds and removes Impalads from a minicluster. Change-Id: If47508728124076f3b9200c27cffc989f7a4f188 --- M be/src/runtime/exec-env.cc M be/src/scheduling/admission-controller-test.cc M be/src/scheduling/admission-controller.cc M be/src/scheduling/admission-controller.h M common/thrift/ImpalaInternalService.thrift M common/thrift/generate_metrics.py M common/thrift/metrics.json M fe/src/main/java/org/apache/impala/util/RequestPoolService.java M fe/src/test/java/org/apache/impala/util/TestRequestPoolService.java M fe/src/test/resources/fair-scheduler-test.xml M fe/src/test/resources/fair-scheduler-test2.xml M fe/src/test/resources/llama-site-test.xml M fe/src/test/resources/llama-site-test2.xml M testdata/workloads/functional-query/queries/QueryTest/admission-max-min-mem-limits.test M testdata/workloads/functional-query/queries/QueryTest/admission-reject-mem-estimate.test M testdata/workloads/functional-query/queries/QueryTest/admission-reject-min-reservation.test M tests/common/impala_cluster.py M tests/custom_cluster/test_admission_controller.py M tests/webserver/test_web_pages.py M www/admission_controller.tmpl 20 files changed, 1,395 insertions(+), 302 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/07/13307/15 -- To view, visit http://gerrit.cloudera.org:8080/13307 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: If47508728124076f3b9200c27cffc989f7a4f188 Gerrit-Change-Number: 13307 Gerrit-PatchSet: 15 Gerrit-Owner: Andrew Sherman Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-8536: Add Scalable Pool Configuration to Admission Controller.
Lars Volker has posted comments on this change. ( http://gerrit.cloudera.org:8080/13307 ) Change subject: IMPALA-8536: Add Scalable Pool Configuration to Admission Controller. .. Patch Set 14: (2 comments) http://gerrit.cloudera.org:8080/#/c/13307/13/tests/custom_cluster/test_admission_controller.py File tests/custom_cluster/test_admission_controller.py: http://gerrit.cloudera.org:8080/#/c/13307/13/tests/custom_cluster/test_admission_controller.py@1045 PS13, Line 1045: ion > I see it bumped in L656 but not here. Given it has been used throughout the I just noticed that the parameter is seconds, so 100 should be perfectly fine. http://gerrit.cloudera.org:8080/#/c/13307/14/tests/custom_cluster/test_admission_controller.py File tests/custom_cluster/test_admission_controller.py: http://gerrit.cloudera.org:8080/#/c/13307/14/tests/custom_cluster/test_admission_controller.py@656 PS14, Line 656: STMT = "select sleep(1000)" I realized that these are seconds, so it's entirely fine to keep 100. -- To view, visit http://gerrit.cloudera.org:8080/13307 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: If47508728124076f3b9200c27cffc989f7a4f188 Gerrit-Change-Number: 13307 Gerrit-PatchSet: 14 Gerrit-Owner: Andrew Sherman Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Mon, 17 Jun 2019 19:58:01 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8536: Add Scalable Pool Configuration to Admission Controller.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13307 ) Change subject: IMPALA-8536: Add Scalable Pool Configuration to Admission Controller. .. Patch Set 14: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/3641/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/13307 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: If47508728124076f3b9200c27cffc989f7a4f188 Gerrit-Change-Number: 13307 Gerrit-PatchSet: 14 Gerrit-Owner: Andrew Sherman Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Mon, 17 Jun 2019 17:46:38 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8536: Add Scalable Pool Configuration to Admission Controller.
Lars Volker has posted comments on this change. ( http://gerrit.cloudera.org:8080/13307 ) Change subject: IMPALA-8536: Add Scalable Pool Configuration to Admission Controller. .. Patch Set 13: (1 comment) http://gerrit.cloudera.org:8080/#/c/13307/13/tests/webserver/test_web_pages.py File tests/webserver/test_web_pages.py: http://gerrit.cloudera.org:8080/#/c/13307/13/tests/webserver/test_web_pages.py@508 PS13, Line 508: assert pool_config['max_running_queries_derived'] > This is a really primitive test to say those metrics exist. I think if they exist but at 0, these tests will fail. If the goal is only to asset that they exist, then "key in pool_config" is the proper way. -- To view, visit http://gerrit.cloudera.org:8080/13307 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: If47508728124076f3b9200c27cffc989f7a4f188 Gerrit-Change-Number: 13307 Gerrit-PatchSet: 13 Gerrit-Owner: Andrew Sherman Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Mon, 17 Jun 2019 17:42:42 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8536: Add Scalable Pool Configuration to Admission Controller.
Lars Volker has posted comments on this change. ( http://gerrit.cloudera.org:8080/13307 ) Change subject: IMPALA-8536: Add Scalable Pool Configuration to Admission Controller. .. Patch Set 14: Code-Review+2 (5 comments) Only had some nits in the test, otherwise LGTM. http://gerrit.cloudera.org:8080/#/c/13307/13/be/src/runtime/exec-env.cc File be/src/runtime/exec-env.cc: http://gerrit.cloudera.org:8080/#/c/13307/13/be/src/runtime/exec-env.cc@214 PS13, Line 214: request_pool_service_.get(), metrics_.get(), configured_backend_address_)); > I think the indent is 4 spaces. I redid it again with clang-format and got Ah, I see, thanks for checking. Historically we only indented once, so "request" would start under "new". I am much in favor of using tooling for this so lets go with clang-format. http://gerrit.cloudera.org:8080/#/c/13307/13/be/src/scheduling/admission-controller.h File be/src/scheduling/admission-controller.h: http://gerrit.cloudera.org:8080/#/c/13307/13/be/src/scheduling/admission-controller.h@677 PS13, Line 677: oop. > I wrote something that is hopefully better. Much clearer, thanks http://gerrit.cloudera.org:8080/#/c/13307/13/be/src/scheduling/admission-controller.cc File be/src/scheduling/admission-controller.cc: http://gerrit.cloudera.org:8080/#/c/13307/13/be/src/scheduling/admission-controller.cc@1083 PS13, Line 1083: // fairness between impalads. > Before calling GetMaxToDequeue we have Good point, it looks like it relies on the dequeue_cv_ logic to propagate state through the statestore. http://gerrit.cloudera.org:8080/#/c/13307/13/tests/custom_cluster/test_admission_controller.py File tests/custom_cluster/test_admission_controller.py: http://gerrit.cloudera.org:8080/#/c/13307/13/tests/custom_cluster/test_admission_controller.py@1045 PS13, Line 1045: ion > Done I see it bumped in L656 but not here. Given it has been used throughout the file for a while it's probably safe as it is. http://gerrit.cloudera.org:8080/#/c/13307/14/tests/custom_cluster/test_admission_controller.py File tests/custom_cluster/test_admission_controller.py: http://gerrit.cloudera.org:8080/#/c/13307/14/tests/custom_cluster/test_admission_controller.py@965 PS14, Line 965: passed mem_limit values passed nit: remove one "passed"? -- To view, visit http://gerrit.cloudera.org:8080/13307 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: If47508728124076f3b9200c27cffc989f7a4f188 Gerrit-Change-Number: 13307 Gerrit-PatchSet: 14 Gerrit-Owner: Andrew Sherman Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Mon, 17 Jun 2019 17:27:31 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8536: Add Scalable Pool Configuration to Admission Controller.
Andrew Sherman has posted comments on this change. (
http://gerrit.cloudera.org:8080/13307 )
Change subject: IMPALA-8536: Add Scalable Pool Configuration to Admission
Controller.
..
Patch Set 14:
(32 comments)
Thanks Lars for the suggestions. I've done most if them...
http://gerrit.cloudera.org:8080/#/c/13307/13/be/src/runtime/exec-env.cc
File be/src/runtime/exec-env.cc:
http://gerrit.cloudera.org:8080/#/c/13307/13/be/src/runtime/exec-env.cc@214
PS13, Line 214: request_pool_service_.get(), metrics_.get(),
configured_backend_address_));
> nit: continue indent at 4 spaces
I think the indent is 4 spaces. I redid it again with clang-format and got the
same thing. So I'm saying this is OK.
http://gerrit.cloudera.org:8080/#/c/13307/13/be/src/scheduling/admission-controller-test.cc
File be/src/scheduling/admission-controller-test.cc:
http://gerrit.cloudera.org:8080/#/c/13307/13/be/src/scheduling/admission-controller-test.cc@50
PS13, Line 50: int6
> nit: int64_t
Done
http://gerrit.cloudera.org:8080/#/c/13307/13/be/src/scheduling/admission-controller-test.cc@98
PS13, Line 98:
> This comment might not be true if the caller passed a different value
deleted stale comment, thanks
http://gerrit.cloudera.org:8080/#/c/13307/13/be/src/scheduling/admission-controller-test.cc@236
PS13, Line 236:
> If you use a FlagSaver in the test class, this can be simplified to just wr
Done
http://gerrit.cloudera.org:8080/#/c/13307/13/be/src/scheduling/admission-controller.h
File be/src/scheduling/admission-controller.h:
http://gerrit.cloudera.org:8080/#/c/13307/13/be/src/scheduling/admission-controller.h@30
PS13, Line 30: #include "common/status.h"
> Add directory, sort into list below
Done
http://gerrit.cloudera.org:8080/#/c/13307/13/be/src/scheduling/admission-controller.h@395
PS13, Line 395: Update
> nit: Updates (here and below)
Done
http://gerrit.cloudera.org:8080/#/c/13307/13/be/src/scheduling/admission-controller.h@660
PS13, Line 660: Return
> Returns (see surrounding code and style guide)
Done
http://gerrit.cloudera.org:8080/#/c/13307/13/be/src/scheduling/admission-controller.h@660
PS13, Line 660: .
> Stale comment?
Done
http://gerrit.cloudera.org:8080/#/c/13307/13/be/src/scheduling/admission-controller.h@667
PS13, Line 667: .
: static int64_
> Stale comment?
Done
http://gerrit.cloudera.org:8080/#/c/13307/13/be/src/scheduling/admission-controller.h@677
PS13, Line 677: oop.
> "at once" seems a bit murky, but I understand that there's no "per time int
I wrote something that is hopefully better.
http://gerrit.cloudera.org:8080/#/c/13307/13/be/src/scheduling/admission-controller.h@694
PS13, Line 694: can be queued in
> typo
Done
http://gerrit.cloudera.org:8080/#/c/13307/13/be/src/scheduling/admission-controller.h@694
PS13, Line 694:
: static int64_t GetM
> stale comment?
Done
http://gerrit.cloudera.org:8080/#/c/13307/13/be/src/scheduling/admission-controller.cc
File be/src/scheduling/admission-controller.cc:
http://gerrit.cloudera.org:8080/#/c/13307/13/be/src/scheduling/admission-controller.cc@422
PS13, Line 422:
> other places have a space before the opening (
Done
http://gerrit.cloudera.org:8080/#/c/13307/13/be/src/scheduling/admission-controller.cc@492
PS13, Line 492: bool has_available_mem_resources =
> nit: The indent looks strange, wrap the whole block in {} if the "if" condi
Done
http://gerrit.cloudera.org:8080/#/c/13307/13/be/src/scheduling/admission-controller.cc@587
PS13, Line 587: if (cluster_mem_to_admit > max_mem) {
> nit: continue at 4 spaces (see above)
As above I am confused, this seems like 4 spaces and clang-format is happy
http://gerrit.cloudera.org:8080/#/c/13307/13/be/src/scheduling/admission-controller.cc@1037
PS13, Line 1037: // better policy once we have better test scenarios.
> nit: continue indent at 4 spaces
I have drunk the clang-format kool-aid and this looks fine
http://gerrit.cloudera.org:8080/#/c/13307/13/be/src/scheduling/admission-controller.cc@1072
PS13, Line 1072: if (!queue.empty()) {
> nit: fix indent
clang-format is happy, I am happy
http://gerrit.cloudera.org:8080/#/c/13307/13/be/src/scheduling/admission-controller.cc@1083
PS13, Line 1083: // fairness between impalads.
> I suspect that this DCHECK could fail if there's a local inrush of queries
Before calling GetMaxToDequeue we have
DCHECK_GE(stats->agg_num_queued(), stats->local_stats().num_queued);
so probably this is just a similar check.
http://gerrit.cloudera.org:8080/#/c/13307/13/be/src/scheduling/admission-controller.cc@1084
PS13, Line 1084: double queue_size_ratio =
static_cast(stats->local_stats().num_queued)
> I think this comment might be wrong/stale: if local_num_queued == agg_num_q
I was intimidated by this TODO and just left it alone.
Now I have deleted it.
[Impala-ASF-CR] IMPALA-8536: Add Scalable Pool Configuration to Admission Controller.
Andrew Sherman has uploaded a new patch set (#14). ( http://gerrit.cloudera.org:8080/13307 ) Change subject: IMPALA-8536: Add Scalable Pool Configuration to Admission Controller. .. IMPALA-8536: Add Scalable Pool Configuration to Admission Controller. Add 3 configuration parameters to Admission Controller that scale with the number of hosts in the resource pool. These parameters are specified to the Impalad through the -llama_site_path flag which points to a Llama XML configuration file. The new configuration parameters are: + Max Running Queries Multiple - this floating point number is multiplied by the current total number of executors at runtime to give the maximum number of concurrently running queries allowed in the pool. This calculation is rounded up to the nearest integer so the result will always be at least one as long as the parameter is non-zero. + Max Queued Queries Multiple - this floating point number is multiplied by the current total number of executors at runtime to give the maximum number of queries that can be queued in the pool. This calculation is rounded up to the nearest integer so the result will always be at least one as long as the parameter is non-zero. + Max Memory Multiple - this number of bytes is multiplied by the current total number of executors at runtime to give the maximum memory available across the cluster for the pool. If any of these parameters have zero value then they will be ignored. In this case the corresponding non-scalable parameters will be used, if they are set. The new parameters are exposed through the webui. At various points in the code Admission Controller looks at the Pool Config objects to find non-scalable parameters such as the max number of queries that can run in the pool. These access have been encapsulated in functions that return the scalable version of the configuration value if the new scalable parameters are being used. Diagnostic messages are enhanced to show the origin of the encapsulated parameters. TESTING All end-to-end tests are running clean with ASAN. The unit test admission-controller-test.cc has been expanded to test the newly added code. Added an end-to-end test that adds and removes Impalads from a minicluster. Change-Id: If47508728124076f3b9200c27cffc989f7a4f188 --- M be/src/runtime/exec-env.cc M be/src/scheduling/admission-controller-test.cc M be/src/scheduling/admission-controller.cc M be/src/scheduling/admission-controller.h M common/thrift/ImpalaInternalService.thrift M common/thrift/generate_metrics.py M common/thrift/metrics.json M fe/src/main/java/org/apache/impala/util/RequestPoolService.java M fe/src/test/java/org/apache/impala/util/TestRequestPoolService.java M fe/src/test/resources/fair-scheduler-test.xml M fe/src/test/resources/fair-scheduler-test2.xml M fe/src/test/resources/llama-site-test.xml M fe/src/test/resources/llama-site-test2.xml M testdata/workloads/functional-query/queries/QueryTest/admission-max-min-mem-limits.test M testdata/workloads/functional-query/queries/QueryTest/admission-reject-mem-estimate.test M testdata/workloads/functional-query/queries/QueryTest/admission-reject-min-reservation.test M tests/common/impala_cluster.py M tests/custom_cluster/test_admission_controller.py M tests/webserver/test_web_pages.py M www/admission_controller.tmpl 20 files changed, 1,393 insertions(+), 302 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/07/13307/14 -- To view, visit http://gerrit.cloudera.org:8080/13307 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: If47508728124076f3b9200c27cffc989f7a4f188 Gerrit-Change-Number: 13307 Gerrit-PatchSet: 14 Gerrit-Owner: Andrew Sherman Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-8536: Add Scalable Pool Configuration to Admission Controller.
Lars Volker has posted comments on this change. (
http://gerrit.cloudera.org:8080/13307 )
Change subject: IMPALA-8536: Add Scalable Pool Configuration to Admission
Controller.
..
Patch Set 13:
(32 comments)
Mostly "dotting the i's", but had some questions around the admission
controller logic
http://gerrit.cloudera.org:8080/#/c/13307/13/be/src/runtime/exec-env.cc
File be/src/runtime/exec-env.cc:
http://gerrit.cloudera.org:8080/#/c/13307/13/be/src/runtime/exec-env.cc@214
PS13, Line 214: request_pool_service_.get(), metrics_.get(),
configured_backend_address_));
nit: continue indent at 4 spaces
http://gerrit.cloudera.org:8080/#/c/13307/13/be/src/scheduling/admission-controller-test.cc
File be/src/scheduling/admission-controller-test.cc:
http://gerrit.cloudera.org:8080/#/c/13307/13/be/src/scheduling/admission-controller-test.cc@50
PS13, Line 50: long
nit: int64_t
http://gerrit.cloudera.org:8080/#/c/13307/13/be/src/scheduling/admission-controller-test.cc@98
PS13, Line 98: // say that every host has 200MB, enough that this isn't a
problem.
This comment might not be true if the caller passed a different value
http://gerrit.cloudera.org:8080/#/c/13307/13/be/src/scheduling/admission-controller-test.cc@236
PS13, Line 236: auto fair_flag = ScopedFlagSetter::Make(
If you use a FlagSaver in the test class, this can be simplified to just
writing to the flags. See
https://github.com/apache/impala/blob/master/be/src/runtime/io/data-cache-test.cc#L120
for an example
http://gerrit.cloudera.org:8080/#/c/13307/13/be/src/scheduling/admission-controller.h
File be/src/scheduling/admission-controller.h:
http://gerrit.cloudera.org:8080/#/c/13307/13/be/src/scheduling/admission-controller.h@30
PS13, Line 30: #include "cluster-membership-mgr.h"
Add directory, sort into list below
http://gerrit.cloudera.org:8080/#/c/13307/13/be/src/scheduling/admission-controller.h@395
PS13, Line 395: Update
nit: Updates (here and below)
http://gerrit.cloudera.org:8080/#/c/13307/13/be/src/scheduling/admission-controller.h@660
PS13, Line 660: with the given schedule
Stale comment?
http://gerrit.cloudera.org:8080/#/c/13307/13/be/src/scheduling/admission-controller.h@660
PS13, Line 660: Return
Returns (see surrounding code and style guide)
http://gerrit.cloudera.org:8080/#/c/13307/13/be/src/scheduling/admission-controller.h@667
PS13, Line 667: with the given
: /// schedule.
Stale comment?
http://gerrit.cloudera.org:8080/#/c/13307/13/be/src/scheduling/admission-controller.h@677
PS13, Line 677: at once
"at once" seems a bit murky, but I understand that there's no "per time
interval" limitation here. I actually suspect that the whole max_to_dequeue
limitation has no effect and we might want to remove it altogether. Thoughts?
http://gerrit.cloudera.org:8080/#/c/13307/13/be/src/scheduling/admission-controller.h@694
PS13, Line 694: can run be queued
typo
http://gerrit.cloudera.org:8080/#/c/13307/13/be/src/scheduling/admission-controller.h@694
PS13, Line 694: with the
: /// given schedule.
stale comment?
http://gerrit.cloudera.org:8080/#/c/13307/13/be/src/scheduling/admission-controller.cc
File be/src/scheduling/admission-controller.cc:
http://gerrit.cloudera.org:8080/#/c/13307/13/be/src/scheduling/admission-controller.cc@422
PS13, Line 422: (
other places have a space before the opening (
http://gerrit.cloudera.org:8080/#/c/13307/13/be/src/scheduling/admission-controller.cc@492
PS13, Line 492: schedule, pool_cfg, cluster_size,
not_admitted_reason)) {
nit: The indent looks strange, wrap the whole block in {} if the "if" condition
doesn't fit into a single line?
http://gerrit.cloudera.org:8080/#/c/13307/13/be/src/scheduling/admission-controller.cc@587
PS13, Line 587: PrintBytes(max_mem),
GetMaxMemForPoolDescription(pool_cfg, cluster_size));
nit: continue at 4 spaces (see above)
http://gerrit.cloudera.org:8080/#/c/13307/13/be/src/scheduling/admission-controller.cc@1037
PS13, Line 1037:*schedule, pool_config, cluster_size, true,
_admitted_reason)) {
nit: continue indent at 4 spaces
http://gerrit.cloudera.org:8080/#/c/13307/13/be/src/scheduling/admission-controller.cc@1072
PS13, Line 1072: GetMaxRequestsForPoolDescription(pool_config,
cluster_size),
nit: fix indent
http://gerrit.cloudera.org:8080/#/c/13307/13/be/src/scheduling/admission-controller.cc@1083
PS13, Line 1083: DCHECK(queue_size_ratio <= 1.0);
I suspect that this DCHECK could fail if there's a local inrush of queries
before the stats get updated. Should we change the max() in the line before to
max(local_queued, agg_queued)?
http://gerrit.cloudera.org:8080/#/c/13307/13/be/src/scheduling/admission-controller.cc@1084
PS13, Line 1084: // TODO: Floating point arithmetic may result in dequeuing
one less request than
I think this comment might be
[Impala-ASF-CR] IMPALA-8536: Add Scalable Pool Configuration to Admission Controller.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13307 ) Change subject: IMPALA-8536: Add Scalable Pool Configuration to Admission Controller. .. Patch Set 13: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/3619/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/13307 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: If47508728124076f3b9200c27cffc989f7a4f188 Gerrit-Change-Number: 13307 Gerrit-PatchSet: 13 Gerrit-Owner: Andrew Sherman Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Thu, 13 Jun 2019 21:39:41 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8536: Add Scalable Pool Configuration to Admission Controller.
Andrew Sherman has posted comments on this change. (
http://gerrit.cloudera.org:8080/13307 )
Change subject: IMPALA-8536: Add Scalable Pool Configuration to Admission
Controller.
..
Patch Set 13:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/13307/12/be/src/scheduling/admission-controller.cc
File be/src/scheduling/admission-controller.cc:
http://gerrit.cloudera.org:8080/#/c/13307/12/be/src/scheduling/admission-controller.cc@1062
PS12, Line 1062: if (PoolLimitsRunningQueriesCount(pool_config)) {
> Could declare this where it's used
Done
--
To view, visit http://gerrit.cloudera.org:8080/13307
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If47508728124076f3b9200c27cffc989f7a4f188
Gerrit-Change-Number: 13307
Gerrit-PatchSet: 13
Gerrit-Owner: Andrew Sherman
Gerrit-Reviewer: Andrew Sherman
Gerrit-Reviewer: Bikramjeet Vig
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker
Gerrit-Reviewer: Tim Armstrong
Gerrit-Comment-Date: Thu, 13 Jun 2019 20:53:43 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8536: Add Scalable Pool Configuration to Admission Controller.
Andrew Sherman has uploaded a new patch set (#13). ( http://gerrit.cloudera.org:8080/13307 ) Change subject: IMPALA-8536: Add Scalable Pool Configuration to Admission Controller. .. IMPALA-8536: Add Scalable Pool Configuration to Admission Controller. Add 3 configuration parameters to Admission Controller that scale with the number of hosts in the resource pool. These parameters are specified to the Impalad through the -llama_site_path flag which points to a Llama XML configuration file. The new configuration parameters are: + Max Running Queries Multiple - this floating point number is multiplied by the current total number of executors at runtime to give the maximum number of concurrently running queries allowed in the pool. This calculation is rounded up to the nearest integer so the result will always be at least one as long as the parameter is non-zero. + Max Queued Queries Multiple - this floating point number is multiplied by the current total number of executors at runtime to give the maximum number of queries that can be queued in the pool. This calculation is rounded up to the nearest integer so the result will always be at least one as long as the parameter is non-zero. + Max Memory Multiple - this number of bytes is multiplied by the current total number of executors at runtime to give the maximum memory available across the cluster for the pool. If any of these parameters have zero value then they will be ignored. In this case the corresponding non-scalable parameters will be used, if they are set. The new parameters are exposed through the webui. At various points in the code Admission Controller looks at the Pool Config objects to find non-scalable parameters such as the max number of queries that can run in the pool. These access have been encapsulated in functions that return the scalable version of the configuration value if the new scalable parameters are being used. Diagnostic messages are enhanced to show the origin of the encapsulated parameters. TESTING All end-to-end tests are running clean with ASAN. The unit test admission-controller-test.cc has been expanded to test the newly added code. Added an end-to-end test that adds and removes Impalads from a minicluster. Change-Id: If47508728124076f3b9200c27cffc989f7a4f188 --- M be/src/runtime/exec-env.cc M be/src/scheduling/admission-controller-test.cc M be/src/scheduling/admission-controller.cc M be/src/scheduling/admission-controller.h M common/thrift/ImpalaInternalService.thrift M common/thrift/generate_metrics.py M common/thrift/metrics.json M fe/src/main/java/org/apache/impala/util/RequestPoolService.java M fe/src/test/java/org/apache/impala/util/TestRequestPoolService.java M fe/src/test/resources/fair-scheduler-test.xml M fe/src/test/resources/fair-scheduler-test2.xml M fe/src/test/resources/llama-site-test.xml M fe/src/test/resources/llama-site-test2.xml M testdata/workloads/functional-query/queries/QueryTest/admission-max-min-mem-limits.test M testdata/workloads/functional-query/queries/QueryTest/admission-reject-mem-estimate.test M testdata/workloads/functional-query/queries/QueryTest/admission-reject-min-reservation.test M tests/common/impala_cluster.py M tests/custom_cluster/test_admission_controller.py M tests/webserver/test_web_pages.py M www/admission_controller.tmpl 20 files changed, 1,399 insertions(+), 304 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/07/13307/13 -- To view, visit http://gerrit.cloudera.org:8080/13307 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: If47508728124076f3b9200c27cffc989f7a4f188 Gerrit-Change-Number: 13307 Gerrit-PatchSet: 13 Gerrit-Owner: Andrew Sherman Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-8536: Add Scalable Pool Configuration to Admission Controller.
Lars Volker has posted comments on this change. ( http://gerrit.cloudera.org:8080/13307 ) Change subject: IMPALA-8536: Add Scalable Pool Configuration to Admission Controller. .. Patch Set 12: (1 comment) http://gerrit.cloudera.org:8080/#/c/13307/12/be/src/scheduling/admission-controller.cc File be/src/scheduling/admission-controller.cc: http://gerrit.cloudera.org:8080/#/c/13307/12/be/src/scheduling/admission-controller.cc@1062 PS12, Line 1062: int64_t total_available; Could declare this where it's used -- To view, visit http://gerrit.cloudera.org:8080/13307 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: If47508728124076f3b9200c27cffc989f7a4f188 Gerrit-Change-Number: 13307 Gerrit-PatchSet: 12 Gerrit-Owner: Andrew Sherman Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Thu, 13 Jun 2019 19:07:03 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8536: Add Scalable Pool Configuration to Admission Controller.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13307 ) Change subject: IMPALA-8536: Add Scalable Pool Configuration to Admission Controller. .. Patch Set 11: Verified+1 -- To view, visit http://gerrit.cloudera.org:8080/13307 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: If47508728124076f3b9200c27cffc989f7a4f188 Gerrit-Change-Number: 13307 Gerrit-PatchSet: 11 Gerrit-Owner: Andrew Sherman Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Thu, 13 Jun 2019 01:42:58 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8536: Add Scalable Pool Configuration to Admission Controller.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13307 ) Change subject: IMPALA-8536: Add Scalable Pool Configuration to Admission Controller. .. Patch Set 12: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/3588/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/13307 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: If47508728124076f3b9200c27cffc989f7a4f188 Gerrit-Change-Number: 13307 Gerrit-PatchSet: 12 Gerrit-Owner: Andrew Sherman Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Wed, 12 Jun 2019 22:15:56 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8536: Add Scalable Pool Configuration to Admission Controller.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13307 ) Change subject: IMPALA-8536: Add Scalable Pool Configuration to Admission Controller. .. Patch Set 11: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/3585/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/13307 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: If47508728124076f3b9200c27cffc989f7a4f188 Gerrit-Change-Number: 13307 Gerrit-PatchSet: 11 Gerrit-Owner: Andrew Sherman Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Wed, 12 Jun 2019 20:53:56 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8536: Add Scalable Pool Configuration to Admission Controller.
Andrew Sherman has posted comments on this change. ( http://gerrit.cloudera.org:8080/13307 ) Change subject: IMPALA-8536: Add Scalable Pool Configuration to Admission Controller. .. Patch Set 12: (1 comment) http://gerrit.cloudera.org:8080/#/c/13307/11/be/src/scheduling/admission-controller.cc File be/src/scheduling/admission-controller.cc: http://gerrit.cloudera.org:8080/#/c/13307/11/be/src/scheduling/admission-controller.cc@1426 PS11, Line 1426: ca > nit: can be Done -- To view, visit http://gerrit.cloudera.org:8080/13307 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: If47508728124076f3b9200c27cffc989f7a4f188 Gerrit-Change-Number: 13307 Gerrit-PatchSet: 12 Gerrit-Owner: Andrew Sherman Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Wed, 12 Jun 2019 20:50:19 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8536: Add Scalable Pool Configuration to Admission Controller.
Andrew Sherman has uploaded a new patch set (#12). ( http://gerrit.cloudera.org:8080/13307 ) Change subject: IMPALA-8536: Add Scalable Pool Configuration to Admission Controller. .. IMPALA-8536: Add Scalable Pool Configuration to Admission Controller. Add 3 configuration parameters to Admission Controller that scale with the number of hosts in the resource pool. These parameters are specified to the Impalad through the -llama_site_path flag which points to a Llama XML configuration file. The new configuration parameters are: + Max Running Queries Multiple - this floating point number is multiplied by the current total number of executors at runtime to give the maximum number of concurrently running queries allowed in the pool. This calculation is rounded up to the nearest integer so the result will always be at least one as long as the parameter is non-zero. + Max Queued Queries Multiple - this floating point number is multiplied by the current total number of executors at runtime to give the maximum number of queries that can be queued in the pool. This calculation is rounded up to the nearest integer so the result will always be at least one as long as the parameter is non-zero. + Max Memory Multiple - this number of bytes is multiplied by the current total number of executors at runtime to give the maximum memory available across the cluster for the pool. If any of these parameters have zero value then they will be ignored. In this case the corresponding non-scalable parameters will be used, if they are set. The new parameters are exposed through the webui. At various points in the code Admission Controller looks at the Pool Config objects to find non-scalable parameters such as the max number of queries that can run in the pool. These access have been encapsulated in functions that return the scalable version of the configuration value if the new scalable parameters are being used. Diagnostic messages are enhanced to show the origin of the encapsulated parameters. TESTING All end-to-end tests are running clean with ASAN. The unit test admission-controller-test.cc has been expanded to test the newly added code. Added an end-to-end test that adds and removes Impalads from a minicluster. Change-Id: If47508728124076f3b9200c27cffc989f7a4f188 --- M be/src/runtime/exec-env.cc M be/src/scheduling/admission-controller-test.cc M be/src/scheduling/admission-controller.cc M be/src/scheduling/admission-controller.h M common/thrift/ImpalaInternalService.thrift M common/thrift/generate_metrics.py M common/thrift/metrics.json M fe/src/main/java/org/apache/impala/util/RequestPoolService.java M fe/src/test/java/org/apache/impala/util/TestRequestPoolService.java M fe/src/test/resources/fair-scheduler-test.xml M fe/src/test/resources/fair-scheduler-test2.xml M fe/src/test/resources/llama-site-test.xml M fe/src/test/resources/llama-site-test2.xml M testdata/workloads/functional-query/queries/QueryTest/admission-max-min-mem-limits.test M testdata/workloads/functional-query/queries/QueryTest/admission-reject-mem-estimate.test M testdata/workloads/functional-query/queries/QueryTest/admission-reject-min-reservation.test M tests/common/impala_cluster.py M tests/custom_cluster/test_admission_controller.py M tests/webserver/test_web_pages.py M www/admission_controller.tmpl 20 files changed, 1,400 insertions(+), 304 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/07/13307/12 -- To view, visit http://gerrit.cloudera.org:8080/13307 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: If47508728124076f3b9200c27cffc989f7a4f188 Gerrit-Change-Number: 13307 Gerrit-PatchSet: 12 Gerrit-Owner: Andrew Sherman Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-8536: Add Scalable Pool Configuration to Admission Controller.
Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/13307 ) Change subject: IMPALA-8536: Add Scalable Pool Configuration to Admission Controller. .. Patch Set 11: Code-Review+1 (2 comments) Looks good to me, I'll let Lars +2 http://gerrit.cloudera.org:8080/#/c/13307/3/be/src/scheduling/admission-controller.cc File be/src/scheduling/admission-controller.cc: http://gerrit.cloudera.org:8080/#/c/13307/3/be/src/scheduling/admission-controller.cc@1403 PS3, Line 1403: string, pair In HasAvailableMemResources() we effectively check the query against both yup you are right, the per host admit_mem_limit would prevent that case. http://gerrit.cloudera.org:8080/#/c/13307/11/be/src/scheduling/admission-controller.cc File be/src/scheduling/admission-controller.cc: http://gerrit.cloudera.org:8080/#/c/13307/11/be/src/scheduling/admission-controller.cc@1426 PS11, Line 1426: is nit: can be -- To view, visit http://gerrit.cloudera.org:8080/13307 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: If47508728124076f3b9200c27cffc989f7a4f188 Gerrit-Change-Number: 13307 Gerrit-PatchSet: 11 Gerrit-Owner: Andrew Sherman Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Wed, 12 Jun 2019 20:41:34 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8536: Add Scalable Pool Configuration to Admission Controller.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13307 ) Change subject: IMPALA-8536: Add Scalable Pool Configuration to Admission Controller. .. Patch Set 11: Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/4459/ DRY_RUN=true -- To view, visit http://gerrit.cloudera.org:8080/13307 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: If47508728124076f3b9200c27cffc989f7a4f188 Gerrit-Change-Number: 13307 Gerrit-PatchSet: 11 Gerrit-Owner: Andrew Sherman Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Wed, 12 Jun 2019 20:15:32 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8536: Add Scalable Pool Configuration to Admission Controller.
Andrew Sherman has uploaded a new patch set (#11). ( http://gerrit.cloudera.org:8080/13307 ) Change subject: IMPALA-8536: Add Scalable Pool Configuration to Admission Controller. .. IMPALA-8536: Add Scalable Pool Configuration to Admission Controller. Add 3 configuration parameters to Admission Controller that scale with the number of hosts in the resource pool. These parameters are specified to the Impalad through the -llama_site_path flag which points to a Llama XML configuration file. The new configuration parameters are: + Max Running Queries Multiple - this floating point number is multiplied by the current total number of executors at runtime to give the maximum number of concurrently running queries allowed in the pool. This calculation is rounded up to the nearest integer so the result will always be at least one as long as the parameter is non-zero. + Max Queued Queries Multiple - this floating point number is multiplied by the current total number of executors at runtime to give the maximum number of queries that can be queued in the pool. This calculation is rounded up to the nearest integer so the result will always be at least one as long as the parameter is non-zero. + Max Memory Multiple - this number of bytes is multiplied by the current total number of executors at runtime to give the maximum memory available across the cluster for the pool. If any of these parameters have zero value then they will be ignored. In this case the corresponding non-scalable parameters will be used, if they are set. The new parameters are exposed through the webui. At various points in the code Admission Controller looks at the Pool Config objects to find non-scalable parameters such as the max number of queries that can run in the pool. These access have been encapsulated in functions that return the scalable version of the configuration value if the new scalable parameters are being used. Diagnostic messages are enhanced to show the origin of the encapsulated parameters. TESTING All end-to-end tests are running clean with ASAN. The unit test admission-controller-test.cc has been expanded to test the newly added code. Added an end-to-end test that adds and removes Impalads from a minicluster. Change-Id: If47508728124076f3b9200c27cffc989f7a4f188 --- M be/src/runtime/exec-env.cc M be/src/scheduling/admission-controller-test.cc M be/src/scheduling/admission-controller.cc M be/src/scheduling/admission-controller.h M common/thrift/ImpalaInternalService.thrift M common/thrift/generate_metrics.py M common/thrift/metrics.json M fe/src/main/java/org/apache/impala/util/RequestPoolService.java M fe/src/test/java/org/apache/impala/util/TestRequestPoolService.java M fe/src/test/resources/fair-scheduler-test.xml M fe/src/test/resources/fair-scheduler-test2.xml M fe/src/test/resources/llama-site-test.xml M fe/src/test/resources/llama-site-test2.xml M testdata/workloads/functional-query/queries/QueryTest/admission-max-min-mem-limits.test M testdata/workloads/functional-query/queries/QueryTest/admission-reject-mem-estimate.test M testdata/workloads/functional-query/queries/QueryTest/admission-reject-min-reservation.test M tests/common/impala_cluster.py M tests/custom_cluster/test_admission_controller.py M tests/webserver/test_web_pages.py M www/admission_controller.tmpl 20 files changed, 1,400 insertions(+), 304 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/07/13307/11 -- To view, visit http://gerrit.cloudera.org:8080/13307 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: If47508728124076f3b9200c27cffc989f7a4f188 Gerrit-Change-Number: 13307 Gerrit-PatchSet: 11 Gerrit-Owner: Andrew Sherman Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-8536: Add Scalable Pool Configuration to Admission Controller.
Andrew Sherman has posted comments on this change. ( http://gerrit.cloudera.org:8080/13307 ) Change subject: IMPALA-8536: Add Scalable Pool Configuration to Admission Controller. .. Patch Set 10: (1 comment) New patch coming to remove the log file I included in my change http://gerrit.cloudera.org:8080/#/c/13307/10/logs/cluster/impalad.INFO File logs/cluster/impalad.INFO: http://gerrit.cloudera.org:8080/#/c/13307/10/logs/cluster/impalad.INFO@1 PS10, Line 1: impalad.asherman-desktop.asherman.log.INFO.20190611-162407.11081 This file should not be here :-( -- To view, visit http://gerrit.cloudera.org:8080/13307 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: If47508728124076f3b9200c27cffc989f7a4f188 Gerrit-Change-Number: 13307 Gerrit-PatchSet: 10 Gerrit-Owner: Andrew Sherman Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Wed, 12 Jun 2019 20:02:59 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8536: Add Scalable Pool Configuration to Admission Controller.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13307 ) Change subject: IMPALA-8536: Add Scalable Pool Configuration to Admission Controller. .. Patch Set 10: Build Failed https://jenkins.impala.io/job/gerrit-code-review-checks/3580/ : Initial code review checks failed. See linked job for details on the failure. -- To view, visit http://gerrit.cloudera.org:8080/13307 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: If47508728124076f3b9200c27cffc989f7a4f188 Gerrit-Change-Number: 13307 Gerrit-PatchSet: 10 Gerrit-Owner: Andrew Sherman Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Wed, 12 Jun 2019 18:55:42 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8536: Add Scalable Pool Configuration to Admission Controller.
Andrew Sherman has uploaded a new patch set (#10). ( http://gerrit.cloudera.org:8080/13307 ) Change subject: IMPALA-8536: Add Scalable Pool Configuration to Admission Controller. .. IMPALA-8536: Add Scalable Pool Configuration to Admission Controller. Add 3 configuration parameters to Admission Controller that scale with the number of hosts in the resource pool. These parameters are specified to the Impalad through the -llama_site_path flag which points to a Llama XML configuration file. The new configuration parameters are: + Max Running Queries Multiple - this floating point number is multiplied by the current total number of executors at runtime to give the maximum number of concurrently running queries allowed in the pool. This calculation is rounded up to the nearest integer so the result will always be at least one as long as the parameter is non-zero. + Max Queued Queries Multiple - this floating point number is multiplied by the current total number of executors at runtime to give the maximum number of queries that can be queued in the pool. This calculation is rounded up to the nearest integer so the result will always be at least one as long as the parameter is non-zero. + Max Memory Multiple - this number of bytes is multiplied by the current total number of executors at runtime to give the maximum memory available across the cluster for the pool. If any of these parameters have zero value then they will be ignored. In this case the corresponding non-scalable parameters will be used, if they are set. The new parameters are exposed through the webui. At various points in the code Admission Controller looks at the Pool Config objects to find non-scalable parameters such as the max number of queries that can run in the pool. These access have been encapsulated in functions that return the scalable version of the configuration value if the new scalable parameters are being used. Diagnostic messages are enhanced to show the origin of the encapsulated parameters. TESTING All end-to-end tests are running clean with ASAN. The unit test admission-controller-test.cc has been expanded to test the newly added code. Added an end-to-end test that adds and removes Impalads from a minicluster. Change-Id: If47508728124076f3b9200c27cffc989f7a4f188 --- M be/src/runtime/exec-env.cc M be/src/scheduling/admission-controller-test.cc M be/src/scheduling/admission-controller.cc M be/src/scheduling/admission-controller.h M common/thrift/ImpalaInternalService.thrift M common/thrift/generate_metrics.py M common/thrift/metrics.json M fe/src/main/java/org/apache/impala/util/RequestPoolService.java M fe/src/test/java/org/apache/impala/util/TestRequestPoolService.java M fe/src/test/resources/fair-scheduler-test.xml M fe/src/test/resources/fair-scheduler-test2.xml M fe/src/test/resources/llama-site-test.xml M fe/src/test/resources/llama-site-test2.xml A logs/cluster/impalad.INFO M testdata/workloads/functional-query/queries/QueryTest/admission-max-min-mem-limits.test M testdata/workloads/functional-query/queries/QueryTest/admission-reject-mem-estimate.test M testdata/workloads/functional-query/queries/QueryTest/admission-reject-min-reservation.test M tests/common/impala_cluster.py M tests/custom_cluster/test_admission_controller.py M tests/webserver/test_web_pages.py M www/admission_controller.tmpl 21 files changed, 1,401 insertions(+), 304 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/07/13307/10 -- To view, visit http://gerrit.cloudera.org:8080/13307 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: If47508728124076f3b9200c27cffc989f7a4f188 Gerrit-Change-Number: 13307 Gerrit-PatchSet: 10 Gerrit-Owner: Andrew Sherman Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-8536: Add Scalable Pool Configuration to Admission Controller.
Andrew Sherman has posted comments on this change. (
http://gerrit.cloudera.org:8080/13307 )
Change subject: IMPALA-8536: Add Scalable Pool Configuration to Admission
Controller.
..
Patch Set 10:
(17 comments)
Thanks for the reviews
http://gerrit.cloudera.org:8080/#/c/13307/8/be/src/scheduling/admission-controller-test.cc
File be/src/scheduling/admission-controller-test.cc:
http://gerrit.cloudera.org:8080/#/c/13307/8/be/src/scheduling/admission-controller-test.cc@317
PS8, Line 317: CheckRoundingForPool(ac, /*expected*/ 3, /*parameter*/ 0.3,
/*num hosts*/ 10
> nit: repeated test
Done
http://gerrit.cloudera.org:8080/#/c/13307/8/be/src/scheduling/admission-controller-test.cc@323
PS8, Line 323: CheckRoundingForPool(ac, /*expected*/ 1, /*parameter*/
100, /*num hosts*/ 100);
> how about another one: CheckRoundingForPool(ac, /*expected*/ 2, /*parameter
Thanks
http://gerrit.cloudera.org:8080/#/c/13307/8/be/src/scheduling/admission-controller-test.cc@327
PS8, Line 327: CanAdmitRequest
> this test is only testing the functionality inside HasAvailableMemResources
Done
http://gerrit.cloudera.org:8080/#/c/13307/8/be/src/scheduling/admission-controller-test.cc@353
PS8, Line 353: // Check that the query can be admitted.
: string not_admitted_reason;
: ASSERT_TRUE(admission_controller->CanAdmitRequest(
: *query_schedule, config_d, host_count, true,
_admitted_reason));
:
: // The query scales with cluster size of 1000.
: host_count = 1000;
: QuerySchedule* query_schedule1000 =
: MakeQuerySchedule(QUEUE_D, config_d, host_count, 30L *
MEGABYTE);
> nit: would it make sense if in the first case host_count=2 the query cannot
That would be a nice test but the memory needed does simply scale with the
number of hosts.
http://gerrit.cloudera.org:8080/#/c/13307/9/be/src/scheduling/admission-controller.h
File be/src/scheduling/admission-controller.h:
http://gerrit.cloudera.org:8080/#/c/13307/9/be/src/scheduling/admission-controller.h@639
PS9, Line 639: const TPoolConfig& pool_cfg, int64_t cluster_size,
std::string* reason);
> I think the method name should reflect that the config now depends on the c
Done
http://gerrit.cloudera.org:8080/#/c/13307/3/be/src/scheduling/admission-controller.cc
File be/src/scheduling/admission-controller.cc:
http://gerrit.cloudera.org:8080/#/c/13307/3/be/src/scheduling/admission-controller.cc@1403
PS3, Line 1403: string, pair pool_config.max_memory_multiple represents the max memory per node allocate
In HasAvailableMemResources() we effectively check the query against both
case 1: the max pool memory
case 2: the per host admit_mem_limit (from
QuerySchedule.per_backend_mem_to_admit).
So I'm not sure we will overadmit.
I could extend IsPoolConfigValid to check for this case at the cost of another
loop through the backend exec params. Do you think that would be useful?
It is slightly confusing as max_memory_multiple is used to calculate an
aggregate memory size, whereas admit_mem_limit could in theory be different on
each host, e.g. perhaps a coordinator might have more memory.
http://gerrit.cloudera.org:8080/#/c/13307/8/be/src/scheduling/admission-controller.cc
File be/src/scheduling/admission-controller.cc:
http://gerrit.cloudera.org:8080/#/c/13307/8/be/src/scheduling/admission-controller.cc@402
PS8, Line 402: Resources(const Que
> I'm in favor of naming this one cluster_size. That should imply that it's t
Done
http://gerrit.cloudera.org:8080/#/c/13307/8/be/src/scheduling/admission-controller.cc@484
PS8, Line 484: stats->local_stats().num_queued,
GetStalenessDetailLocked(" "));
: return false;
: } else if (max_requests >= 0 && stats->agg_num_running() >=
max_requests) {
: *not_admitted_reason = Substitute(QUEUED_NUM_RUNNING,
stats->agg_num_running(),
: max_requests, GetMaxRequestsForPoolDescription(pool_cfg,
cluster_size),
: GetStalenessDetailLocked(" "));
> nit: should we add info similar to GetMaxQueuedForPoolDescription here for
Done for QUEUED_NUM_RUNNING, but QUEUED_QUEUE_NOT_EMPTY seems OK as is.
http://gerrit.cloudera.org:8080/#/c/13307/8/be/src/scheduling/admission-controller.cc@1129
PS8, Line 1129: pool_cfg.min_query_mem_limit, max_mem);
> We could replace this call with its return value, given that the pool_cfg u
Done
http://gerrit.cloudera.org:8080/#/c/13307/8/be/src/scheduling/admission-controller.cc@1422
PS8, Line 1422:
: // If we exclude the quiescing executors, and a query at the
head of the queue is
: // scheduled to run on those, then the scaled down limits of
the pool would prevent it
: // from being admitted, and hold up the rest of the queue
> nit: how about=> If we exclude the
[Impala-ASF-CR] IMPALA-8536: Add Scalable Pool Configuration to Admission Controller.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13307 ) Change subject: IMPALA-8536: Add Scalable Pool Configuration to Admission Controller. .. Patch Set 9: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/3574/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/13307 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: If47508728124076f3b9200c27cffc989f7a4f188 Gerrit-Change-Number: 13307 Gerrit-PatchSet: 9 Gerrit-Owner: Andrew Sherman Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Wed, 12 Jun 2019 01:16:17 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8536: Add Scalable Pool Configuration to Admission Controller.
Andrew Sherman has uploaded a new patch set (#9). ( http://gerrit.cloudera.org:8080/13307 ) Change subject: IMPALA-8536: Add Scalable Pool Configuration to Admission Controller. .. IMPALA-8536: Add Scalable Pool Configuration to Admission Controller. Add 3 configuration parameters to Admission Controller that scale with the number of hosts in the resource pool. These parameters are specified to the Impalad through the -llama_site_path flag which points to a Llama XML configuration file. The new configuration parameters are: + Max Running Queries Multiple - this floating point number is multiplied by the current total number of executors at runtime to give the maximum number of concurrently running queries allowed in the pool. This calculation is rounded up to the nearest integer so the result will always be at least one as long as the parameter is non-zero. + Max Queued Queries Multiple - this floating point number is multiplied by the current total number of executors at runtime to give the maximum number of queries that can be queued in the pool. This calculation is rounded up to the nearest integer so the result will always be at least one as long as the parameter is non-zero. + Max Memory Multiple - this number of bytes is multiplied by the current total number of executors at runtime to give the maximum memory available across the cluster for the pool. If any of these parameters have zero value then they will be ignored. In this case the corresponding non-scalable parameters will be used, if they are set. The new parameters are exposed through the webui. At various points in the code Admission Controller looks at the Pool Config objects to find non-scalable parameters such as the max number of queries that can run in the pool. These access have been encapsulated in functions that return the scalable version of the configuration value if the new scalable parameters are being used. Diagnostic messages are enhanced to show the origin of the encapsulated parameters. TESTING All end-to-end tests are running clean with ASAN. The unit test admission-controller-test.cc has been expanded to test the newly added code. Added an end-to-end test that adds and removes Impalads from a minicluster. Change-Id: If47508728124076f3b9200c27cffc989f7a4f188 --- M be/src/runtime/exec-env.cc M be/src/scheduling/admission-controller-test.cc M be/src/scheduling/admission-controller.cc M be/src/scheduling/admission-controller.h M common/thrift/ImpalaInternalService.thrift M common/thrift/generate_metrics.py M common/thrift/metrics.json M fe/src/main/java/org/apache/impala/util/RequestPoolService.java M fe/src/test/java/org/apache/impala/util/TestRequestPoolService.java M fe/src/test/resources/fair-scheduler-test.xml M fe/src/test/resources/fair-scheduler-test2.xml M fe/src/test/resources/llama-site-test.xml M fe/src/test/resources/llama-site-test2.xml M testdata/workloads/functional-query/queries/QueryTest/admission-max-min-mem-limits.test M testdata/workloads/functional-query/queries/QueryTest/admission-reject-mem-estimate.test M testdata/workloads/functional-query/queries/QueryTest/admission-reject-min-reservation.test M tests/common/impala_cluster.py M tests/custom_cluster/test_admission_controller.py M tests/webserver/test_web_pages.py M www/admission_controller.tmpl 20 files changed, 1,408 insertions(+), 305 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/07/13307/9 -- To view, visit http://gerrit.cloudera.org:8080/13307 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: If47508728124076f3b9200c27cffc989f7a4f188 Gerrit-Change-Number: 13307 Gerrit-PatchSet: 9 Gerrit-Owner: Andrew Sherman Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-8536: Add Scalable Pool Configuration to Admission Controller.
Bikramjeet Vig has posted comments on this change. (
http://gerrit.cloudera.org:8080/13307 )
Change subject: IMPALA-8536: Add Scalable Pool Configuration to Admission
Controller.
..
Patch Set 8:
(16 comments)
http://gerrit.cloudera.org:8080/#/c/13307/3//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/13307/3//COMMIT_MSG@15
PS3, Line 15: Max Running
> OK, this is complicated, like all naming things.
The reason I was vouching for requests because then it will be consistent with
the other config param (max_requests) which it is directly related to, both of
these params will be written to the same file, so having them consistent
locally will be less confusing.
I do however completely agree that "queries" will be more clearer so lets stick
to that.
http://gerrit.cloudera.org:8080/#/c/13307/8/be/src/scheduling/admission-controller-test.cc
File be/src/scheduling/admission-controller-test.cc:
http://gerrit.cloudera.org:8080/#/c/13307/8/be/src/scheduling/admission-controller-test.cc@317
PS8, Line 317: CheckRoundingForPool(ac, /*expected*/ 0, /*parameter*/ 0, /*num
hosts*/ 100)
nit: repeated test
http://gerrit.cloudera.org:8080/#/c/13307/8/be/src/scheduling/admission-controller-test.cc@323
PS8, Line 323: CheckRoundingForPool(ac, /*expected*/ 1, /*parameter*/
100, /*num hosts*/ 100);
how about another one: CheckRoundingForPool(ac, /*expected*/ 2, /*parameter*/
.5, /*num hosts*/ 3);
http://gerrit.cloudera.org:8080/#/c/13307/8/be/src/scheduling/admission-controller-test.cc@327
PS8, Line 327: CanAdmitRequest
this test is only testing the functionality inside HasAvailableMemResources(),
can you add another test which manipulates the pool_stats to test out the other
num_running queries scalable param.
http://gerrit.cloudera.org:8080/#/c/13307/8/be/src/scheduling/admission-controller-test.cc@353
PS8, Line 353: // Check that the query can be admitted.
: string not_admitted_reason;
: ASSERT_TRUE(admission_controller->CanAdmitRequest(
: *query_schedule, config_d, host_count, true,
_admitted_reason));
:
: // The query scales with cluster size of 1000.
: host_count = 1000;
: ASSERT_TRUE(admission_controller->CanAdmitRequest(
: *query_schedule, config_d, host_count, true,
_admitted_reason));
nit: would it make sense if in the first case host_count=2 the query cannot be
updated and in the second case it can because it scaled with the increase in
cluster size
http://gerrit.cloudera.org:8080/#/c/13307/8/be/src/scheduling/admission-controller.h
File be/src/scheduling/admission-controller.h:
http://gerrit.cloudera.org:8080/#/c/13307/8/be/src/scheduling/admission-controller.h@294
PS8, Line 294: /// Pointer to the cluster membership manager.
nit: mention that it is not owned by this class
http://gerrit.cloudera.org:8080/#/c/13307/3/be/src/scheduling/admission-controller.cc
File be/src/scheduling/admission-controller.cc:
http://gerrit.cloudera.org:8080/#/c/13307/3/be/src/scheduling/admission-controller.cc@1403
PS3, Line 1403: string, pair I don;t understand what you mean here. Do you mean QuerySchedule.per_backen
pool_config.max_memory_multiple represents the max memory per node allocated to
this resource pool. Now if that value is more than an executor's
TBackendDescriptor.admit_mem_limit then we are effectively over-committing
memory for this pool on that node
http://gerrit.cloudera.org:8080/#/c/13307/8/be/src/scheduling/admission-controller.cc
File be/src/scheduling/admission-controller.cc:
http://gerrit.cloudera.org:8080/#/c/13307/8/be/src/scheduling/admission-controller.cc@484
PS8, Line 484: *not_admitted_reason = Substitute(QUEUED_QUEUE_NOT_EMPTY,
: stats->local_stats().num_queued,
GetStalenessDetailLocked(" "));
: return false;
: } else if (max_requests >= 0 && stats->agg_num_running() >=
max_requests) {
: *not_admitted_reason = Substitute(QUEUED_NUM_RUNNING,
stats->agg_num_running(),
: max_requests, GetStalenessDetailLocked(" "));
nit: should we add info similar to GetMaxQueuedForPoolDescription here for both
QUEUED_QUEUE_NOT_EMPTY and QUEUED_NUM_RUNNING?
http://gerrit.cloudera.org:8080/#/c/13307/8/be/src/scheduling/admission-controller.cc@1422
PS8, Line 1422: If this
: // value could change between the time a query is queued and
the time that we try
: // to admit it from the queue then it might be impossible to
run the query at the
: // head of the queue, which might block admission forever.
nit: how about=> If we exclude the quiescing executors and a query at the head
of the queue is scheduled to run on those, then the scaled down limits of the
pool would prevent it from being admitted and hold up the rest of the queue
[Impala-ASF-CR] IMPALA-8536: Add Scalable Pool Configuration to Admission Controller.
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/13307 ) Change subject: IMPALA-8536: Add Scalable Pool Configuration to Admission Controller. .. Patch Set 8: Code-Review+1 (1 comment) http://gerrit.cloudera.org:8080/#/c/13307/6/be/src/scheduling/admission-controller.cc File be/src/scheduling/admission-controller.cc: http://gerrit.cloudera.org:8080/#/c/13307/6/be/src/scheduling/admission-controller.cc@1445 PS6, Line 1445: > Yes! I see you added a regression test - thank you! -- To view, visit http://gerrit.cloudera.org:8080/13307 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: If47508728124076f3b9200c27cffc989f7a4f188 Gerrit-Change-Number: 13307 Gerrit-PatchSet: 8 Gerrit-Owner: Andrew Sherman Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Mon, 10 Jun 2019 21:37:28 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8536: Add Scalable Pool Configuration to Admission Controller.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13307 ) Change subject: IMPALA-8536: Add Scalable Pool Configuration to Admission Controller. .. Patch Set 8: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/3555/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/13307 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: If47508728124076f3b9200c27cffc989f7a4f188 Gerrit-Change-Number: 13307 Gerrit-PatchSet: 8 Gerrit-Owner: Andrew Sherman Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Mon, 10 Jun 2019 17:51:06 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8536: Add Scalable Pool Configuration to Admission Controller.
Andrew Sherman has uploaded a new patch set (#8). ( http://gerrit.cloudera.org:8080/13307 ) Change subject: IMPALA-8536: Add Scalable Pool Configuration to Admission Controller. .. IMPALA-8536: Add Scalable Pool Configuration to Admission Controller. Add 3 configuration parameters to Admission Controller that scale with the number of hosts in the resource pool. These parameters are specified to the Impalad through the -llama_site_path flag which points to a Llama XML configuration file. The new configuration parameters are: + Max Running Queries Multiple - this floating point number is multiplied by the current total number of executors at runtime to give the maximum number of concurrently running queries allowed in the pool. This calculation is rounded up to the nearest integer so the result will always be at least one as long as the parameter is non-zero. + Max Queued Queries Multiple - this floating point number is multiplied by the current total number of executors at runtime to give the maximum number of queries that can be queued in the pool. This calculation is rounded up to the nearest integer so the result will always be at least one as long as the parameter is non-zero. + Max Memory Multiple - this number of bytes is multiplied by the current total number of executors at runtime to give the maximum memory available across the cluster for the pool. If any of these parameters have zero value then they will be ignored. In this case the corresponding non-scalable parameters will be used, if they are set. The new parameters are exposed through the webui. At various points in the code Admission Controller looks at the Pool Config objects to find non-scalable parameters such as the max number of queries that can run in the pool. These access have been encapsulated in functions that return the scalable version of the configuration value if the new scalable parameters are being used. Diagnostic messages are enhanced to show the origin of the encapsulated parameters. TESTING All end-to-end tests are running clean with ASAN. The unit test admission-controller-test.cc has been expanded to test the newly added code. Added an end-to-end test that adds and removes Impalads from a minicluster. Change-Id: If47508728124076f3b9200c27cffc989f7a4f188 --- M be/src/runtime/exec-env.cc M be/src/scheduling/admission-controller-test.cc M be/src/scheduling/admission-controller.cc M be/src/scheduling/admission-controller.h M common/thrift/ImpalaInternalService.thrift M common/thrift/generate_metrics.py M common/thrift/metrics.json M fe/src/main/java/org/apache/impala/util/RequestPoolService.java M fe/src/test/java/org/apache/impala/util/TestRequestPoolService.java M fe/src/test/resources/fair-scheduler-test.xml M fe/src/test/resources/fair-scheduler-test2.xml M fe/src/test/resources/llama-site-test.xml M fe/src/test/resources/llama-site-test2.xml M testdata/workloads/functional-query/queries/QueryTest/admission-max-min-mem-limits.test M testdata/workloads/functional-query/queries/QueryTest/admission-reject-mem-estimate.test M testdata/workloads/functional-query/queries/QueryTest/admission-reject-min-reservation.test M tests/common/impala_cluster.py M tests/custom_cluster/test_admission_controller.py M tests/webserver/test_web_pages.py M www/admission_controller.tmpl 20 files changed, 1,338 insertions(+), 297 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/07/13307/8 -- To view, visit http://gerrit.cloudera.org:8080/13307 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: If47508728124076f3b9200c27cffc989f7a4f188 Gerrit-Change-Number: 13307 Gerrit-PatchSet: 8 Gerrit-Owner: Andrew Sherman Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-8536: Add Scalable Pool Configuration to Admission Controller.
Andrew Sherman has posted comments on this change. ( http://gerrit.cloudera.org:8080/13307 ) Change subject: IMPALA-8536: Add Scalable Pool Configuration to Admission Controller. .. Patch Set 7: In fact I am removing my ClusterMembershipMgr change entirely. Reviewers please wait for a new patch -- To view, visit http://gerrit.cloudera.org:8080/13307 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: If47508728124076f3b9200c27cffc989f7a4f188 Gerrit-Change-Number: 13307 Gerrit-PatchSet: 7 Gerrit-Owner: Andrew Sherman Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Fri, 07 Jun 2019 19:03:51 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8536: Add Scalable Pool Configuration to Admission Controller.
Andrew Sherman has posted comments on this change. ( http://gerrit.cloudera.org:8080/13307 ) Change subject: IMPALA-8536: Add Scalable Pool Configuration to Admission Controller. .. Patch Set 7: (1 comment) I'm not happy with the ClusterMembershipMgr change, I'm working on doing things more efficiently. http://gerrit.cloudera.org:8080/#/c/13307/7/be/src/scheduling/cluster-membership-mgr.cc File be/src/scheduling/cluster-membership-mgr.cc: http://gerrit.cloudera.org:8080/#/c/13307/7/be/src/scheduling/cluster-membership-mgr.cc@278 PS7, Line 278: if (!entry.second.is_quiescing) count++; This can be done more efficiently, I'm working on an update -- To view, visit http://gerrit.cloudera.org:8080/13307 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: If47508728124076f3b9200c27cffc989f7a4f188 Gerrit-Change-Number: 13307 Gerrit-PatchSet: 7 Gerrit-Owner: Andrew Sherman Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Fri, 07 Jun 2019 17:41:20 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8536: Add Scalable Pool Configuration to Admission Controller.
Andrew Sherman has posted comments on this change. ( http://gerrit.cloudera.org:8080/13307 ) Change subject: IMPALA-8536: Add Scalable Pool Configuration to Admission Controller. .. Patch Set 7: (21 comments) Addressing review comments, main changes are: Switched to using ClusterMembershipMgr to get cluster membership. Enhanced ClusterMembershipMgr to add a count of non-quiescing backends to the Snapshot. Added a simple unit test for AdmissionControoler::PoolDisabled. Added metrics for derived configuration values. http://gerrit.cloudera.org:8080/#/c/13307/3/be/src/scheduling/admission-controller-test.cc File be/src/scheduling/admission-controller-test.cc: http://gerrit.cloudera.org:8080/#/c/13307/3/be/src/scheduling/admission-controller-test.cc@279 PS3, Line 279: // AdmissionController. > Consider breaking this up into a few more targeted tests, I think it makes I have tidied up and added comments. http://gerrit.cloudera.org:8080/#/c/13307/6/be/src/scheduling/admission-controller-test.cc File be/src/scheduling/admission-controller-test.cc: http://gerrit.cloudera.org:8080/#/c/13307/6/be/src/scheduling/admission-controller-test.cc@71 PS6, Line 71: > Comment could be more descriptive. Done http://gerrit.cloudera.org:8080/#/c/13307/6/be/src/scheduling/admission-controller-test.cc@74 PS6, Line 74: QuerySchedule* MakeQuerySchedule(string request_pool_name, TPoolConfig& config, > DCHECK_GT Done http://gerrit.cloudera.org:8080/#/c/13307/6/be/src/scheduling/admission-controller-test.cc@175 PS6, Line 175: } > Extra spaces after /// Done http://gerrit.cloudera.org:8080/#/c/13307/6/be/src/scheduling/admission-controller-test.cc@204 PS6, Line 204: } > Comment is a little uninformative. Maybe "Make an AdmissionController with Done http://gerrit.cloudera.org:8080/#/c/13307/5/be/src/scheduling/admission-controller.h File be/src/scheduling/admission-controller.h: http://gerrit.cloudera.org:8080/#/c/13307/5/be/src/scheduling/admission-controller.h@663 PS5, Line 663: > nit: Uses a Done http://gerrit.cloudera.org:8080/#/c/13307/5/be/src/scheduling/admission-controller.h@664 PS5, Line 664: pool is configured. : static std::string GetMaxMemForPoolDescription( : const TPoolConfig& pool_config, int64_t latest_cluster_size); > nit: not sure if this detail is relevant there. maybe remove this? Done http://gerrit.cloudera.org:8080/#/c/13307/3/be/src/scheduling/admission-controller.h File be/src/scheduling/admission-controller.h: http://gerrit.cloudera.org:8080/#/c/13307/3/be/src/scheduling/admission-controller.h@603 PS3, Line 603: > Not a common term but just reviewing IMPALA-8460 drilled into me that the n Ah now I understand what you didn't like. Going with your original suggestion 'latest_cluster_size' http://gerrit.cloudera.org:8080/#/c/13307/3/be/src/scheduling/admission-controller.cc File be/src/scheduling/admission-controller.cc: http://gerrit.cloudera.org:8080/#/c/13307/3/be/src/scheduling/admission-controller.cc@1396 PS3, Line 1396: uge( > I thought about this more and I think not counting the quiescing backends w Thanks for raising the question.I am now not counting the quiescing backends. http://gerrit.cloudera.org:8080/#/c/13307/3/be/src/scheduling/admission-controller.cc@1444 PS3, Line 1444: tu > should be OR (||) YES! http://gerrit.cloudera.org:8080/#/c/13307/5/be/src/scheduling/admission-controller.cc File be/src/scheduling/admission-controller.cc: http://gerrit.cloudera.org:8080/#/c/13307/5/be/src/scheduling/admission-controller.cc@1051 PS5, Line 1051: > nit: outdated comment Done http://gerrit.cloudera.org:8080/#/c/13307/6/be/src/scheduling/admission-controller.cc File be/src/scheduling/admission-controller.cc: http://gerrit.cloudera.org:8080/#/c/13307/6/be/src/scheduling/admission-controller.cc@1390 PS6, Line 1390: metrics_.max_queued_queries_multiple = parent_->metrics_group_->AddDoubleGauge( > I think we need to be a little careful with this being O(cluster size). If Thanks, new implementation uses ClusterMembershipMgr and the value is added to value in ClusterMembershipMgr::Snapshot . http://gerrit.cloudera.org:8080/#/c/13307/6/be/src/scheduling/admission-controller.cc@1445 PS6, Line 1445: > Should this be ||? Since we shouldn't be able to submit queries to a pool i Yes! http://gerrit.cloudera.org:8080/#/c/13307/5/tests/custom_cluster/test_admission_controller.py File tests/custom_cluster/test_admission_controller.py: http://gerrit.cloudera.org:8080/#/c/13307/5/tests/custom_cluster/test_admission_controller.py@994 PS5, Line 994: dmission > nit: Impalads Done http://gerrit.cloudera.org:8080/#/c/13307/5/tests/custom_cluster/test_admission_controller.py@1002 PS5, Line 1002: :param expected_rejection_reason: a string expected to be in the reason for rejection. > nit: by convention, we usually prefix a helper method with a double undersc Done
[Impala-ASF-CR] IMPALA-8536: Add Scalable Pool Configuration to Admission Controller.
Andrew Sherman has uploaded a new patch set (#7). ( http://gerrit.cloudera.org:8080/13307 ) Change subject: IMPALA-8536: Add Scalable Pool Configuration to Admission Controller. .. IMPALA-8536: Add Scalable Pool Configuration to Admission Controller. Add 3 configuration parameters to Admission Controller that scale with the number of hosts in the resource pool. These parameters are specified to the Impalad through the -llama_site_path flag which points to a Llama XML configuration file. The new configuration parameters are: + Max Running Queries Multiple - this floating point number is multiplied by the current total number of executors at runtime to give the maximum number of concurrently running queries allowed in the pool. This calculation is rounded up to the nearest integer so the result will always be at least one as long as the parameter is non-zero. + Max Queued Queries Multiple - this floating point number is multiplied by the current total number of executors at runtime to give the maximum number of queries that can be queued in the pool. This calculation is rounded up to the nearest integer so the result will always be at least one as long as the parameter is non-zero. + Max Memory Multiple - this number of bytes is multiplied by the current total number of executors at runtime to give the maximum memory available across the cluster for the pool. If any of these parameters have zero value then they will be ignored. In this case the corresponding non-scalable parameters will be used, if they are set. The new parameters are exposed through the webui. At various points in the code Admission Controller looks at the Pool Config objects to find non-scalable parameters such as the max number of queries that can run in the pool. These access have been encapsulated in functions that return the scalable version of the configuration value if the new scalable parameters are being used. Diagnostic messages are enhanced to show the origin of the encapsulated parameters. TESTING All end-to-end tests are running clean with ASAN. The unit test admission-controller-test.cc has been expanded to test the newly added code. Added an end-to-end test that adds and removes Impalads from a minicluster. Change-Id: If47508728124076f3b9200c27cffc989f7a4f188 --- M be/src/runtime/exec-env.cc M be/src/scheduling/admission-controller-test.cc M be/src/scheduling/admission-controller.cc M be/src/scheduling/admission-controller.h M be/src/scheduling/cluster-membership-mgr-test.cc M be/src/scheduling/cluster-membership-mgr.cc M be/src/scheduling/cluster-membership-mgr.h M common/thrift/ImpalaInternalService.thrift M common/thrift/generate_metrics.py M common/thrift/metrics.json M fe/src/main/java/org/apache/impala/util/RequestPoolService.java M fe/src/test/java/org/apache/impala/util/TestRequestPoolService.java M fe/src/test/resources/fair-scheduler-test.xml M fe/src/test/resources/fair-scheduler-test2.xml M fe/src/test/resources/llama-site-test.xml M fe/src/test/resources/llama-site-test2.xml M testdata/workloads/functional-query/queries/QueryTest/admission-max-min-mem-limits.test M testdata/workloads/functional-query/queries/QueryTest/admission-reject-mem-estimate.test M testdata/workloads/functional-query/queries/QueryTest/admission-reject-min-reservation.test M tests/common/impala_cluster.py M tests/custom_cluster/test_admission_controller.py M tests/webserver/test_web_pages.py M www/admission_controller.tmpl 23 files changed, 1,356 insertions(+), 298 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/07/13307/7 -- To view, visit http://gerrit.cloudera.org:8080/13307 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: If47508728124076f3b9200c27cffc989f7a4f188 Gerrit-Change-Number: 13307 Gerrit-PatchSet: 7 Gerrit-Owner: Andrew Sherman Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-8536: Add Scalable Pool Configuration to Admission Controller.
Tim Armstrong has posted comments on this change. (
http://gerrit.cloudera.org:8080/13307 )
Change subject: IMPALA-8536: Add Scalable Pool Configuration to Admission
Controller.
..
Patch Set 6:
(8 comments)
http://gerrit.cloudera.org:8080/#/c/13307/6/be/src/scheduling/admission-controller-test.cc
File be/src/scheduling/admission-controller-test.cc:
http://gerrit.cloudera.org:8080/#/c/13307/6/be/src/scheduling/admission-controller-test.cc@71
PS6, Line 71: /// Build a QuerySchedule object.
Comment could be more descriptive.
http://gerrit.cloudera.org:8080/#/c/13307/6/be/src/scheduling/admission-controller-test.cc@74
PS6, Line 74: DCHECK(num_hosts > 0);
DCHECK_GT
http://gerrit.cloudera.org:8080/#/c/13307/6/be/src/scheduling/admission-controller-test.cc@175
PS6, Line 175: /// Check the calculations made by GetMaxQueuedForPool and
GetMaxRequestsForPool are
Extra spaces after ///
http://gerrit.cloudera.org:8080/#/c/13307/6/be/src/scheduling/admission-controller-test.cc@204
PS6, Line 204: /// Make an AdmissionController
Comment is a little uninformative. Maybe "Make an AdmissionController with some
dummy parameters" or something like that.
http://gerrit.cloudera.org:8080/#/c/13307/6/be/src/scheduling/admission-controller.h
File be/src/scheduling/admission-controller.h:
http://gerrit.cloudera.org:8080/#/c/13307/6/be/src/scheduling/admission-controller.h@583
PS6, Line 583: int64_t cluster_size_snapshot, bool admit_from_queue,
string* not_admitted_reason);
We generally use std::string in .h since the rule is that headers shouldn't
import things into the global namespace. Some headers we depend on don't follow
this rule, so std::string got into the global namespace somehow, but we
shouldn't depend on that.
http://gerrit.cloudera.org:8080/#/c/13307/6/be/src/scheduling/admission-controller.cc
File be/src/scheduling/admission-controller.cc:
http://gerrit.cloudera.org:8080/#/c/13307/6/be/src/scheduling/admission-controller.cc@1390
PS6, Line 1390: int64_t AdmissionController::GetClusterSize() {
I think we need to be a little careful with this being O(cluster size). If it
was used in the wrong context the runtime could end up blowing up. I think it's
ok with the current usage pattern, but it might be worth caching this value in
ClusterMembershipMgr::Snapshot to avoid repeated recomputations.
This is kind-of premature optimisation, but I think any perf issues might rear
their head in inconvenient situations, e.g. very high query concurrency.
http://gerrit.cloudera.org:8080/#/c/13307/6/be/src/scheduling/admission-controller.cc@1445
PS6, Line 1445: &&
Should this be ||? Since we shouldn't be able to submit queries to a pool if
either it has 0 request or 0 memory.
http://gerrit.cloudera.org:8080/#/c/13307/6/common/thrift/ImpalaInternalService.thrift
File common/thrift/ImpalaInternalService.thrift:
http://gerrit.cloudera.org:8080/#/c/13307/6/common/thrift/ImpalaInternalService.thrift@695
PS6, Line 695: :
inconsistent whitespace before colon
--
To view, visit http://gerrit.cloudera.org:8080/13307
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If47508728124076f3b9200c27cffc989f7a4f188
Gerrit-Change-Number: 13307
Gerrit-PatchSet: 6
Gerrit-Owner: Andrew Sherman
Gerrit-Reviewer: Andrew Sherman
Gerrit-Reviewer: Bikramjeet Vig
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker
Gerrit-Reviewer: Tim Armstrong
Gerrit-Comment-Date: Mon, 03 Jun 2019 23:24:13 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8536: Add Scalable Pool Configuration to Admission Controller.
Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/13307 ) Change subject: IMPALA-8536: Add Scalable Pool Configuration to Admission Controller. .. Patch Set 6: (15 comments) http://gerrit.cloudera.org:8080/#/c/13307/5/be/src/scheduling/admission-controller.h File be/src/scheduling/admission-controller.h: http://gerrit.cloudera.org:8080/#/c/13307/5/be/src/scheduling/admission-controller.h@663 PS5, Line 663: Use a nit: Uses a http://gerrit.cloudera.org:8080/#/c/13307/5/be/src/scheduling/admission-controller.h@664 PS5, Line 664: This is based on the : /// max_requests_estimate limit and the current queue size. We will attempt to : /// dequeue up to this number of requests until reaching the per-pool memory limit nit: not sure if this detail is relevant there. maybe remove this? http://gerrit.cloudera.org:8080/#/c/13307/3/be/src/scheduling/admission-controller.h File be/src/scheduling/admission-controller.h: http://gerrit.cloudera.org:8080/#/c/13307/3/be/src/scheduling/admission-controller.h@603 PS3, Line 603: _size_snapshot, > The ClusterMembershipMgr that will come in IMPALA-8460 has a method GetSnap Not a common term but just reviewing IMPALA-8460 drilled into me that the notion that a snapshot is like a snapshot of some (meta)data. how about cluster_snapshot_size? http://gerrit.cloudera.org:8080/#/c/13307/3/be/src/scheduling/admission-controller.cc File be/src/scheduling/admission-controller.cc: http://gerrit.cloudera.org:8080/#/c/13307/3/be/src/scheduling/admission-controller.cc@1396 PS3, Line 1396: > I originally used the cluster size from QuerySchedule but hit problems with I thought about this more and I think not counting the quiescing backends would be the better alternative since in this case the queries scheduled to run on those backends would not hold up the rest of the queue and in case queries scheduled on lesser num of backends get overadmitted, we can consider those an extension of the case where a query is only scheduled on a subset of backends (another eg would be query only scheduled on coord). http://gerrit.cloudera.org:8080/#/c/13307/3/be/src/scheduling/admission-controller.cc@1444 PS3, Line 1444: rn > should be OR (||) http://gerrit.cloudera.org:8080/#/c/13307/5/be/src/scheduling/admission-controller.cc File be/src/scheduling/admission-controller.cc: http://gerrit.cloudera.org:8080/#/c/13307/5/be/src/scheduling/admission-controller.cc@1051 PS5, Line 1051: absolute limit nit: outdated comment http://gerrit.cloudera.org:8080/#/c/13307/5/tests/custom_cluster/test_admission_controller.py File tests/custom_cluster/test_admission_controller.py: http://gerrit.cloudera.org:8080/#/c/13307/5/tests/custom_cluster/test_admission_controller.py@994 PS5, Line 994: Impalada nit: Impalads http://gerrit.cloudera.org:8080/#/c/13307/5/tests/custom_cluster/test_admission_controller.py@1002 PS5, Line 1002: def check_admission_by_memory(self, expected_admission, expected_rejection_reason=None): nit: by convention, we usually prefix a helper method with a double underscore ( __ ) http://gerrit.cloudera.org:8080/#/c/13307/5/tests/custom_cluster/test_admission_controller.py@1003 PS5, Line 1003: query = "select * from functional.alltypesagg order by int_col limit 1" nit: add method comment http://gerrit.cloudera.org:8080/#/c/13307/5/tests/custom_cluster/test_admission_controller.py@1003 PS5, Line 1003: query = "select * from functional.alltypesagg order by int_col limit 1" might be useful to set the mem_limit too to make this less flaky in case the estimates change in the future. http://gerrit.cloudera.org:8080/#/c/13307/5/tests/custom_cluster/test_admission_controller.py@1015 PS5, Line 1015: assert expected_rejection_reason in rejected_reasons[0] nit: print the profile if this assert fails for easy debugging http://gerrit.cloudera.org:8080/#/c/13307/5/tests/custom_cluster/test_admission_controller.py@1016 PS5, Line 1016: yybecause nit: perhaps you forgot to remove this line? http://gerrit.cloudera.org:8080/#/c/13307/5/tests/custom_cluster/test_admission_controller.py@1025 PS5, Line 1025: Run some queries, find how many were admitted, queued or rejected, and then compare nit: maybe mention that this is to verify if the admission controller correctly enforces query count limits http://gerrit.cloudera.org:8080/#/c/13307/5/tests/custom_cluster/test_admission_controller.py@1037 PS5, Line 1037: assert (expected_num_admitted + expected_num_queued + : expected_num_rejected) == NUM_QUERIES it seems like we can calculate all these expected values from the num of impalads in the cluster, maybe that is the only variable we need to pass to this methods since the logic for expected values is incorporated in this method itself (as the request_pool is constant) .
[Impala-ASF-CR] IMPALA-8536: Add Scalable Pool Configuration to Admission Controller.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13307 ) Change subject: IMPALA-8536: Add Scalable Pool Configuration to Admission Controller. .. Patch Set 6: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/3481/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/13307 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: If47508728124076f3b9200c27cffc989f7a4f188 Gerrit-Change-Number: 13307 Gerrit-PatchSet: 6 Gerrit-Owner: Andrew Sherman Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Mon, 03 Jun 2019 17:29:19 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8536: Add Scalable Pool Configuration to Admission Controller.
Andrew Sherman has uploaded a new patch set (#6). ( http://gerrit.cloudera.org:8080/13307 ) Change subject: IMPALA-8536: Add Scalable Pool Configuration to Admission Controller. .. IMPALA-8536: Add Scalable Pool Configuration to Admission Controller. Add 3 configuration parameters to Admission Controller that scale with the number of hosts in the resource pool. These parameters are specified to the Impalad through the -llama_site_path flag which points to a Llama XML configuration file. The new configuration parameters are: + Max Running Queries Multiple - this floating point number is multiplied by the current total number of executors at runtime to give the maximum number of concurrently running queries allowed in the pool. This calculation is rounded up to the nearest integer so the result will always be at least one as long as the parameter is non-zero. + Max Queued Queries Multiple - this floating point number is multiplied by the current total number of executors at runtime to give the maximum number of queries that can be queued in the pool. This calculation is rounded up to the nearest integer so the result will always be at least one as long as the parameter is non-zero. + Max Memory Multiple - this number of bytes is multiplied by the current total number of executors at runtime to give the maximum memory available across the cluster for the pool. If any of these parameters have zero value then they will be ignored. In this case the corresponding non-scalable parameters will be used, if they are set. The new parameters are exposed through the webui. At various points in the code Admission Controller looks at the Pool Config objects to find non-scalable parameters such as the max number of queries that can run in the pool. These access have been encapsulated in functions that return the scalable version of the configuration value if the new scalable parameters are being used. Diagnostic messages are enhanced to show the origin of the encapsulated parameters. TESTING All end-to-end tests are running clean with ASAN. The unit test admission-controller-test.cc has been expanded to test the newly added code. Added an end-to-end test that adds and removes Impalads from a minicluster. Change-Id: If47508728124076f3b9200c27cffc989f7a4f188 --- M be/src/scheduling/admission-controller-test.cc M be/src/scheduling/admission-controller.cc M be/src/scheduling/admission-controller.h M common/thrift/ImpalaInternalService.thrift M common/thrift/generate_metrics.py M common/thrift/metrics.json M fe/src/main/java/org/apache/impala/util/RequestPoolService.java M fe/src/test/java/org/apache/impala/util/TestRequestPoolService.java M fe/src/test/resources/fair-scheduler-test.xml M fe/src/test/resources/fair-scheduler-test2.xml M fe/src/test/resources/llama-site-test.xml M fe/src/test/resources/llama-site-test2.xml M testdata/workloads/functional-query/queries/QueryTest/admission-max-min-mem-limits.test M testdata/workloads/functional-query/queries/QueryTest/admission-reject-mem-estimate.test M testdata/workloads/functional-query/queries/QueryTest/admission-reject-min-reservation.test M tests/common/impala_cluster.py M tests/custom_cluster/test_admission_controller.py M tests/webserver/test_web_pages.py M www/admission_controller.tmpl 19 files changed, 1,193 insertions(+), 270 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/07/13307/6 -- To view, visit http://gerrit.cloudera.org:8080/13307 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: If47508728124076f3b9200c27cffc989f7a4f188 Gerrit-Change-Number: 13307 Gerrit-PatchSet: 6 Gerrit-Owner: Andrew Sherman Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-8536: Add Scalable Pool Configuration to Admission Controller.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13307 ) Change subject: IMPALA-8536: Add Scalable Pool Configuration to Admission Controller. .. Patch Set 5: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/3469/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/13307 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: If47508728124076f3b9200c27cffc989f7a4f188 Gerrit-Change-Number: 13307 Gerrit-PatchSet: 5 Gerrit-Owner: Andrew Sherman Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Fri, 31 May 2019 22:28:18 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8536: Add Scalable Pool Configuration to Admission Controller.
Andrew Sherman has posted comments on this change. ( http://gerrit.cloudera.org:8080/13307 ) Change subject: IMPALA-8536: Add Scalable Pool Configuration to Admission Controller. .. Patch Set 5: It's actually easy (form a code point of view) to ignore quiescing machines - see see patch set 5. -- To view, visit http://gerrit.cloudera.org:8080/13307 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: If47508728124076f3b9200c27cffc989f7a4f188 Gerrit-Change-Number: 13307 Gerrit-PatchSet: 5 Gerrit-Owner: Andrew Sherman Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Fri, 31 May 2019 21:37:05 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8536: Add Scalable Pool Configuration to Admission Controller.
Andrew Sherman has uploaded a new patch set (#5). ( http://gerrit.cloudera.org:8080/13307 ) Change subject: IMPALA-8536: Add Scalable Pool Configuration to Admission Controller. .. IMPALA-8536: Add Scalable Pool Configuration to Admission Controller. Add 3 configuration parameters to Admission Controller that scale with the number of hosts in the resource pool. These parameters are specified to the Impalad through the -llama_site_path flag which points to a Llama XML configuration file. The new configuration parameters are: + Max Running Queries Multiple - this floating point number is multiplied by the current total number of executors at runtime to give the maximum number of concurrently running queries allowed in the pool. This calculation is rounded up to the nearest integer so the result will always be at least one as long as the parameter is non-zero. + Max Queued Queries Multiple - this floating point number is multiplied by the current total number of executors at runtime to give the maximum number of queries that can be queued in the pool. This calculation is rounded up to the nearest integer so the result will always be at least one as long as the parameter is non-zero. + Max Memory Multiple - this number of bytes is multiplied by the current total number of executors at runtime to give the maximum memory available across the cluster for the pool. If any of these parameters have zero value then they will be ignored. In this case the corresponding non-scalable parameters will be used, if they are set. The new parameters are exposed through the webui. At various points in the code Admission Controller looks at the Pool Config objects to find non-scalable parameters such as the max number of queries that can run in the pool. These access have been encapsulated in functions that return the scalable version of the configuration value if the new scalable parameters are being used. Diagnostic messages are enhanced to show the origin of the encapsulated parameters. TESTING All end-to-end tests are running clean with ASAN. The unit test admission-controller-test.cc has been expanded to test the newly added code. Added an end-to-end test that adds and removes Impalads from a minicluster. Change-Id: If47508728124076f3b9200c27cffc989f7a4f188 --- M be/src/scheduling/admission-controller-test.cc M be/src/scheduling/admission-controller.cc M be/src/scheduling/admission-controller.h M common/thrift/ImpalaInternalService.thrift M common/thrift/generate_metrics.py M common/thrift/metrics.json M fe/src/main/java/org/apache/impala/util/RequestPoolService.java M fe/src/test/java/org/apache/impala/util/TestRequestPoolService.java M fe/src/test/resources/fair-scheduler-test.xml M fe/src/test/resources/fair-scheduler-test2.xml M fe/src/test/resources/llama-site-test.xml M fe/src/test/resources/llama-site-test2.xml M testdata/workloads/functional-query/queries/QueryTest/admission-max-min-mem-limits.test M testdata/workloads/functional-query/queries/QueryTest/admission-reject-mem-estimate.test M testdata/workloads/functional-query/queries/QueryTest/admission-reject-min-reservation.test M tests/common/impala_cluster.py M tests/custom_cluster/test_admission_controller.py M tests/webserver/test_web_pages.py M www/admission_controller.tmpl 19 files changed, 1,193 insertions(+), 270 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/07/13307/5 -- To view, visit http://gerrit.cloudera.org:8080/13307 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: If47508728124076f3b9200c27cffc989f7a4f188 Gerrit-Change-Number: 13307 Gerrit-PatchSet: 5 Gerrit-Owner: Andrew Sherman Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-8536: Add Scalable Pool Configuration to Admission Controller.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13307 ) Change subject: IMPALA-8536: Add Scalable Pool Configuration to Admission Controller. .. Patch Set 4: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/3463/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/13307 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: If47508728124076f3b9200c27cffc989f7a4f188 Gerrit-Change-Number: 13307 Gerrit-PatchSet: 4 Gerrit-Owner: Andrew Sherman Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Fri, 31 May 2019 18:41:08 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8536: Add Scalable Pool Configuration to Admission Controller.
Andrew Sherman has posted comments on this change. ( http://gerrit.cloudera.org:8080/13307 ) Change subject: IMPALA-8536: Add Scalable Pool Configuration to Admission Controller. .. Patch Set 4: (22 comments) Thanks Tim and Bikram for the thorough reviews - see new patch set 4 for fixes. http://gerrit.cloudera.org:8080/#/c/13307/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/13307/1//COMMIT_MSG@21 PS1, Line 21: + Max Queued Queries Multiple - this floating point number is multiplied > I think this is ok for now. I thought about it some more and having one opt Done http://gerrit.cloudera.org:8080/#/c/13307/3//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/13307/3//COMMIT_MSG@14 PS3, Line 14: The new configuration parameters are: > mention that a value of zero means it wont be used and will default to the Thanks http://gerrit.cloudera.org:8080/#/c/13307/3//COMMIT_MSG@15 PS3, Line 15: Max Running > nit: use "Max Requests" to be consistent OK, this is complicated, like all naming things. Yes it is "max_requests" in thrift. But in CM (which is what I am really taking about in the commit msg) it is "Max Running Queries". And in the webui it is "Max concurrent queries" So there is no totally consistent value. I think "queries" is clearer than "requests" but we can talk more! http://gerrit.cloudera.org:8080/#/c/13307/3//COMMIT_MSG@26 PS3, Line 26: this number of bytes is multiplied by the : current total number of executors at runtime to give the maximum : memory available acro > can you elaborate on what this number is like you did for the ones above? A Done http://gerrit.cloudera.org:8080/#/c/13307/3/be/src/scheduling/admission-controller-test.cc File be/src/scheduling/admission-controller-test.cc: http://gerrit.cloudera.org:8080/#/c/13307/3/be/src/scheduling/admission-controller-test.cc@503 PS3, Line 503: stats.local_stats_.num_queued = 10; > ScopedFlagSetter? Done http://gerrit.cloudera.org:8080/#/c/13307/3/be/src/scheduling/admission-controller-test.cc@545 PS3, Line 545: stats.agg_num_queued_ = host_count; > Would it make sense to test that the pool stats were actually modified in t Done http://gerrit.cloudera.org:8080/#/c/13307/3/be/src/scheduling/admission-controller-test.cc@555 PS3, Line 555: } > Would it make sense to test that the pool stats were actually modified in t Done http://gerrit.cloudera.org:8080/#/c/13307/3/be/src/scheduling/admission-controller.h File be/src/scheduling/admission-controller.h: http://gerrit.cloudera.org:8080/#/c/13307/3/be/src/scheduling/admission-controller.h@582 PS3, Line 582: bool CanAdmitRequest(const QuerySchedule& schedule, const TPoolConfig& pool_cfg, > I'd prefer to use int64_t for consistency (reasonable people can differ abo Thanks http://gerrit.cloudera.org:8080/#/c/13307/3/be/src/scheduling/admission-controller.h@603 PS3, Line 603: _size_snapshot, > nit: maybe use something like latest_cluster_size. the snapshot in the name The ClusterMembershipMgr that will come in IMPALA-8460 has a method GetSnapshot() to fetch the cluster membership. The cluster size used by AC will eventually come from there which is why I chose it. What was that made you expect a struct? If this is a common Impala term then I should change it. http://gerrit.cloudera.org:8080/#/c/13307/3/be/src/scheduling/admission-controller.h@661 PS3, Line 661: /// If it can be determined that no queries can currently be run, then zero > It would help to clarify if max_to_dequeue can be 0 if Status is non-OK. If Yes, that is much clearer thanks. http://gerrit.cloudera.org:8080/#/c/13307/3/be/src/scheduling/admission-controller.cc File be/src/scheduling/admission-controller.cc: http://gerrit.cloudera.org:8080/#/c/13307/3/be/src/scheduling/admission-controller.cc@1000 PS3, Line 1000: int64_t max_to_dequeue = : GetMaxToDequeue(queue, stats, pool_config, cluster_size_snapshot); : VLOG_RPC << "Dequeue thread will try to admit " << max_to_dequeue << " requests" :<< ", pool=" << pool_name > nit: if this functionality is completely moved over to a method, maybe we c Done http://gerrit.cloudera.org:8080/#/c/13307/3/be/src/scheduling/admission-controller.cc@1007 PS3, Line 1007: > I agree, would prefer returning max_to_dequeue itself and handling the <= 0 Done http://gerrit.cloudera.org:8080/#/c/13307/3/be/src/scheduling/admission-controller.cc@1007 PS3, Line 1007: > I think the old behaviour had the advantage of logging something if nothing Yes, thanks, I moved this logic below the logging. http://gerrit.cloudera.org:8080/#/c/13307/3/be/src/scheduling/admission-controller.cc@1070 PS3, Line 1070: return min(stats->local_stats().num_queued, > Shouldn't this use the ceil() logic from GetMaxRequestsForPool()? Or at lea Yes, should be a double, this was a
[Impala-ASF-CR] IMPALA-8536: Add Scalable Pool Configuration to Admission Controller.
Andrew Sherman has uploaded a new patch set (#4). ( http://gerrit.cloudera.org:8080/13307 ) Change subject: IMPALA-8536: Add Scalable Pool Configuration to Admission Controller. .. IMPALA-8536: Add Scalable Pool Configuration to Admission Controller. Add 3 configuration parameters to Admission Controller that scale with the number of hosts in the resource pool. These parameters are specified to the Impalad through the -llama_site_path flag which points to a Llama XML configuration file. The new configuration parameters are: + Max Running Queries Multiple - this floating point number is multiplied by the current total number of executors at runtime to give the maximum number of concurrently running queries allowed in the pool. This calculation is rounded up to the nearest integer so the result will always be at least one as long as the parameter is non-zero. + Max Queued Queries Multiple - this floating point number is multiplied by the current total number of executors at runtime to give the maximum number of queries that can be queued in the pool. This calculation is rounded up to the nearest integer so the result will always be at least one as long as the parameter is non-zero. + Max Memory Multiple - this number of bytes is multiplied by the current total number of executors at runtime to give the maximum memory available across the cluster for the pool. If any of these parameters have zero value then they will be ignored. In this case the corresponding non-scalable parameters will be used, if they are set. The new parameters are exposed through the webui. At various points in the code Admission Controller looks at the Pool Config objects to find non-scalable parameters such as the max number of queries that can run in the pool. These access have been encapsulated in functions that return the scalable version of the configuration value if the new scalable parameters are being used. Diagnostic messages are enhanced to show the origin of the encapsulated parameters. TESTING All end-to-end tests are running clean with ASAN. The unit test admission-controller-test.cc has been expanded to test the newly added code. Added an end-to-end test that adds and removes Impalads from a minicluster. Change-Id: If47508728124076f3b9200c27cffc989f7a4f188 --- M be/src/scheduling/admission-controller-test.cc M be/src/scheduling/admission-controller.cc M be/src/scheduling/admission-controller.h M common/thrift/ImpalaInternalService.thrift M common/thrift/generate_metrics.py M common/thrift/metrics.json M fe/src/main/java/org/apache/impala/util/RequestPoolService.java M fe/src/test/java/org/apache/impala/util/TestRequestPoolService.java M fe/src/test/resources/fair-scheduler-test.xml M fe/src/test/resources/fair-scheduler-test2.xml M fe/src/test/resources/llama-site-test.xml M fe/src/test/resources/llama-site-test2.xml M testdata/workloads/functional-query/queries/QueryTest/admission-max-min-mem-limits.test M testdata/workloads/functional-query/queries/QueryTest/admission-reject-mem-estimate.test M testdata/workloads/functional-query/queries/QueryTest/admission-reject-min-reservation.test M tests/common/impala_cluster.py M tests/custom_cluster/test_admission_controller.py M tests/webserver/test_web_pages.py M www/admission_controller.tmpl 19 files changed, 1,187 insertions(+), 270 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/07/13307/4 -- To view, visit http://gerrit.cloudera.org:8080/13307 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: If47508728124076f3b9200c27cffc989f7a4f188 Gerrit-Change-Number: 13307 Gerrit-PatchSet: 4 Gerrit-Owner: Andrew Sherman Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-8536: Add Scalable Pool Configuration to Admission Controller.
Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/13307 ) Change subject: IMPALA-8536: Add Scalable Pool Configuration to Admission Controller. .. Patch Set 3: (12 comments) looks good, just looked at the implementation for now, will look at the tests next http://gerrit.cloudera.org:8080/#/c/13307/3//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/13307/3//COMMIT_MSG@14 PS3, Line 14: The new configuration parameters are: mention that a value of zero means it wont be used and will default to the non-scalable param. http://gerrit.cloudera.org:8080/#/c/13307/3//COMMIT_MSG@15 PS3, Line 15: Max Running nit: use "Max Requests" to be consistent http://gerrit.cloudera.org:8080/#/c/13307/3//COMMIT_MSG@26 PS3, Line 26: the Multiple of the current total number of : running Impalads which gives the maximum memory available across the : cluster for the pool. can you elaborate on what this number is like you did for the ones above? Also mention the unit (bytes). Also update the same in the thrift definition http://gerrit.cloudera.org:8080/#/c/13307/3/be/src/scheduling/admission-controller.h File be/src/scheduling/admission-controller.h: http://gerrit.cloudera.org:8080/#/c/13307/3/be/src/scheduling/admission-controller.h@603 PS3, Line 603: cluster_size_snapshot nit: maybe use something like latest_cluster_size. the snapshot in the name threw me off a bit since i expected a struct or something instead of just the size. Feel free to ignore this comment. http://gerrit.cloudera.org:8080/#/c/13307/3/be/src/scheduling/admission-controller.cc File be/src/scheduling/admission-controller.cc: http://gerrit.cloudera.org:8080/#/c/13307/3/be/src/scheduling/admission-controller.cc@1000 PS3, Line 1000: // Use a heuristic to limit the number of requests we dequeue locally to avoid all : // impalads dequeuing too many requests at the same time. This is based on the : // max_requests_estimate limit and the current queue size. We will attempt to : // dequeue up to this number of requests until reaching the per-pool memory limit. nit: if this functionality is completely moved over to a method, maybe we can mention all this in the method comment and remove it here http://gerrit.cloudera.org:8080/#/c/13307/3/be/src/scheduling/admission-controller.cc@1007 PS3, Line 1007: if (!status.ok()) continue; // to next pool > I think the old behaviour had the advantage of logging something if nothing I agree, would prefer returning max_to_dequeue itself and handling the <= 0 case http://gerrit.cloudera.org:8080/#/c/13307/3/be/src/scheduling/admission-controller.cc@1071 PS3, Line 1071: total_available this can still be less than equal to zero, right? like in case of overadmission. If yes then we can get rid of the branching based on PoolHasFixedRunningQueriesCountLimit http://gerrit.cloudera.org:8080/#/c/13307/3/be/src/scheduling/admission-controller.cc@1121 PS3, Line 1121: "Invalid pool config: the min_query_mem_limit $0 is greater than Seems like now this case can happen if the cluster size changes (an external reason), so calling this an invalid config might not be right. Do you think it makes sense to mention both cases (invalid config and low cluster size) and offer ways to fix it, like we do in REASON_MEM_LIMIT_TOO_LOW_FOR_RESERVATION http://gerrit.cloudera.org:8080/#/c/13307/3/be/src/scheduling/admission-controller.cc@1396 PS3, Line 1396: GetKnownBackends this might be a problem if some backends are shutting down and the query wont be scheduled on them but the max memory used is still high, causing over admission. If instead we only look at the size of the active backends, that would be a problem for queries already scheduled to run on the backends shutting down. @all Do you think it makes sense to handle this transient period right now? It might be a problem if a group of executors have started shutting down at the same time. Another solution could be use to the cluster size from the QuerySchedule itself, but that has its own set of problems. Any thoughts? http://gerrit.cloudera.org:8080/#/c/13307/3/be/src/scheduling/admission-controller.cc@1403 PS3, Line 1403: max_memory_multiple just thinking out loud: should we check if this multiple is greater than the executor.mem_to_admit for each executor because in that case this would be an invalid config. Maybe not, since there is currently nothing preventing us from setting a higher pool_config.max_mem_resources as well. http://gerrit.cloudera.org:8080/#/c/13307/3/be/src/scheduling/admission-controller.cc@1444 PS3, Line 1444: && should be OR (||) http://gerrit.cloudera.org:8080/#/c/13307/3/common/thrift/ImpalaInternalService.thrift File common/thrift/ImpalaInternalService.thrift:
[Impala-ASF-CR] IMPALA-8536: Add Scalable Pool Configuration to Admission Controller.
Tim Armstrong has posted comments on this change. (
http://gerrit.cloudera.org:8080/13307 )
Change subject: IMPALA-8536: Add Scalable Pool Configuration to Admission
Controller.
..
Patch Set 3:
(13 comments)
Overall looks good, I appreciate the refactoring and added unit tests.
http://gerrit.cloudera.org:8080/#/c/13307/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/13307/1//COMMIT_MSG@21
PS1, Line 21: + Max Queued Queries Multiple - this floating point number is
multiplied
> I did think about this but I welcome suggestions and/or decisions from more
I think this is ok for now. I thought about it some more and having one option
doesn't seem to preclude adding a percentage-based option later if we think it
covers another additional use case.
http://gerrit.cloudera.org:8080/#/c/13307/3/be/src/scheduling/admission-controller-test.cc
File be/src/scheduling/admission-controller-test.cc:
http://gerrit.cloudera.org:8080/#/c/13307/3/be/src/scheduling/admission-controller-test.cc@279
PS3, Line 279: TEST_F(AdmissionControllerTest, ScalableConfiguration) {
Consider breaking this up into a few more targeted tests, I think it makes
sense but it's a little hard to wrap my head around the totality of what this
function is testing.
http://gerrit.cloudera.org:8080/#/c/13307/3/be/src/scheduling/admission-controller-test.cc@503
PS3, Line 503: FLAGS_fair_scheduler_allocation_path =
GetResourceFile("fair-scheduler-test2.xml");
ScopedFlagSetter?
http://gerrit.cloudera.org:8080/#/c/13307/3/be/src/scheduling/admission-controller-test.cc@545
PS3, Line 545: pool_stats->UpdateAggregates();
Would it make sense to test that the pool stats were actually modified in the
middle?
http://gerrit.cloudera.org:8080/#/c/13307/3/be/src/scheduling/admission-controller-test.cc@555
PS3, Line 555: CheckPoolStatsEmpty(pool_stats);
Would it make sense to test that the pool stats were actually modified in the
middle?
http://gerrit.cloudera.org:8080/#/c/13307/3/be/src/scheduling/admission-controller.h
File be/src/scheduling/admission-controller.h:
http://gerrit.cloudera.org:8080/#/c/13307/3/be/src/scheduling/admission-controller.h@582
PS3, Line 582: const uint64_t cluster_size_snapshot, bool
admit_from_queue,
I'd prefer to use int64_t for consistency (reasonable people can differ about
whether this is the best practice, but I've found it confusing working on
codebases that mixed signed and unsigned integers heavily). The google C++
style guide does specify this -
https://google.github.io/styleguide/cppguide.html#Integer_Types
Also we don't need a const for arguments passed by value, I find it adds too
much noise for the benefit of preventing the function implementation assigning
its local copy.
http://gerrit.cloudera.org:8080/#/c/13307/3/be/src/scheduling/admission-controller.h@661
PS3, Line 661: /// race to consume all the available resources.
It would help to clarify if max_to_dequeue can be 0 if Status is non-OK. If it
can, I'm wondering if there's value in having two mechanisms to signal "don't
dequeue anything". Maybe if we logged the status, but it doesn't look like we
do.
http://gerrit.cloudera.org:8080/#/c/13307/3/be/src/scheduling/admission-controller.cc
File be/src/scheduling/admission-controller.cc:
http://gerrit.cloudera.org:8080/#/c/13307/3/be/src/scheduling/admission-controller.cc@1007
PS3, Line 1007: if (!status.ok()) continue; // to next pool
I think the old behaviour had the advantage of logging something if nothing was
dequeued. Could be hard to figure out what happened here if this returns
non-OK. Also see my comment about returning an error vs returning 0 in the
header.
http://gerrit.cloudera.org:8080/#/c/13307/3/be/src/scheduling/admission-controller.cc@1070
PS3, Line 1070: pool_config.max_running_queries_multiple;
Shouldn't this use the ceil() logic from GetMaxRequestsForPool()? Or at least
keep it as a double so it's > 0 for the below calculations.
http://gerrit.cloudera.org:8080/#/c/13307/3/be/src/scheduling/admission-controller.cc@1393
PS3, Line 1393: uint64_t AdmissionController::GetClusterSize() {
I like this refactoring, I think it makes things clearer than the previous code
scrounging around in the thrift object.
http://gerrit.cloudera.org:8080/#/c/13307/3/common/thrift/metrics.json
File common/thrift/metrics.json:
http://gerrit.cloudera.org:8080/#/c/13307/3/common/thrift/metrics.json@83
PS3, Line 83: Impalads
executors?
http://gerrit.cloudera.org:8080/#/c/13307/3/common/thrift/metrics.json@93
PS3, Line 93: Impalads
executors?
http://gerrit.cloudera.org:8080/#/c/13307/3/common/thrift/metrics.json@103
PS3, Line 103: Impalads
executors?
--
To view, visit http://gerrit.cloudera.org:8080/13307
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id:
[Impala-ASF-CR] IMPALA-8536: Add Scalable Pool Configuration to Admission Controller.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13307 ) Change subject: IMPALA-8536: Add Scalable Pool Configuration to Admission Controller. .. Patch Set 3: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/3278/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/13307 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: If47508728124076f3b9200c27cffc989f7a4f188 Gerrit-Change-Number: 13307 Gerrit-PatchSet: 3 Gerrit-Owner: Andrew Sherman Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Sat, 18 May 2019 00:27:55 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8536: Add Scalable Pool Configuration to Admission Controller.
Andrew Sherman has uploaded a new patch set (#3). ( http://gerrit.cloudera.org:8080/13307 ) Change subject: IMPALA-8536: Add Scalable Pool Configuration to Admission Controller. .. IMPALA-8536: Add Scalable Pool Configuration to Admission Controller. Add 3 configuration parameters to Admission Controller that scale with the number of hosts in the resource pool. These parameters are specified to the Impalad through the -llama_site_path flag which points to a Llama XML configuration file. The new configuration parameters are: + Max Running Queries Multiple - this floating point number is multiplied by the current total number of running Impalads at runtime to give the maximum number of concurrently running queries allowed in the pool. This calculation is rounded up to the nearest integer so the result will always be at least one as long as the parameter is non-zero. + Max Queued Queries Multiple - this floating point number is multiplied by the current total number of running Impalads at runtime to give the maximum number of queries that can be queued in the pool. This calculation is rounded up to the nearest integer so the result will always be at least one as long as the parameter is non-zero. + Max Memory Multiple - the Multiple of the current total number of running Impalads which gives the maximum memory available across the cluster for the pool. The new parameters are exposed through the webui. At various points in the code Admission Controller looks at the Pool Config objects to find non-scalable parameters such as the max number of queries that can run in the pool. These access have been encapsulated in functions that return the scalable version of the configuration value if the new scalable parameters are being used. Diagnostic messages are enhanced to show the origin of the encapsulated parameters. TESTING All end-to-end tests are running clean with ASAN. The unit test admission-controller-test.cc has been expanded to test the newly added code. Added an end-to-end test that adds and removes Impalads from a minicluster. Change-Id: If47508728124076f3b9200c27cffc989f7a4f188 --- M be/src/scheduling/admission-controller-test.cc M be/src/scheduling/admission-controller.cc M be/src/scheduling/admission-controller.h M common/thrift/ImpalaInternalService.thrift M common/thrift/generate_metrics.py M common/thrift/metrics.json M fe/src/main/java/org/apache/impala/util/RequestPoolService.java M fe/src/test/java/org/apache/impala/util/TestRequestPoolService.java M fe/src/test/resources/fair-scheduler-test.xml M fe/src/test/resources/fair-scheduler-test2.xml M fe/src/test/resources/llama-site-test.xml M fe/src/test/resources/llama-site-test2.xml M testdata/workloads/functional-query/queries/QueryTest/admission-max-min-mem-limits.test M testdata/workloads/functional-query/queries/QueryTest/admission-reject-mem-estimate.test M testdata/workloads/functional-query/queries/QueryTest/admission-reject-min-reservation.test M tests/common/impala_cluster.py M tests/custom_cluster/test_admission_controller.py M tests/webserver/test_web_pages.py M www/admission_controller.tmpl 19 files changed, 1,107 insertions(+), 243 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/07/13307/3 -- To view, visit http://gerrit.cloudera.org:8080/13307 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: If47508728124076f3b9200c27cffc989f7a4f188 Gerrit-Change-Number: 13307 Gerrit-PatchSet: 3 Gerrit-Owner: Andrew Sherman Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Tim Armstrong
