[Impala-ASF-CR] IMPALA-7811: optionally count JVM heap towards process mem limit
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/10928 ) Change subject: IMPALA-7811: optionally count JVM heap towards process mem limit .. Patch Set 15: Verified+1 -- To view, visit http://gerrit.cloudera.org:8080/10928 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I39dd715882a32fc986755d573bd46f0fd9eefbfc Gerrit-Change-Number: 10928 Gerrit-PatchSet: 15 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Pooja Nilangekar Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Tue, 04 Dec 2018 08:20:33 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7811: optionally count JVM heap towards process mem limit
Impala Public Jenkins has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/10928 ) Change subject: IMPALA-7811: optionally count JVM heap towards process mem limit .. IMPALA-7811: optionally count JVM heap towards process mem limit Adds a flag --mem_limit_includes_jvm that alters memory accounting to include the amount of memory we think that the JVM is likely to use. By default this flag is false, so behaviour is unchanged. We're not ready to change the default but I want to check this in to enable experimentation. Two metrics are counted towards the process limit: * The maximum JVM heap size. We count this because the JVM memory consumption can expand up to this threshold at any time. * JVM non-heap committed memory. This can be a non-trivial amount of memory (e.g. I saw 150MB on one production cluster). There isn't a hard upper bound on this memory that I know of but should not grow rapidly. This requires adjustments in a couple of other places: * Admission control previous assumed that all of the process memory limit was available to queries (an assumption that is not strictly true because of untracked memory, etc, but close enough). However, the JVM heap makes a large part of the process limit unusable to queries, so we should only admit up to "process limit - max JVM heap size" per node. * The buffer pool is now a percentage of the remaining process limit after the JVM heap, instead of the total process limit. Currently, end-to-end tests fail if run with this flag for two reasons: * The default JVM heap size is 1/4 of physical memory, which means that essentially all of the process memory limit is consumed by the JVM heaps when we running 3 impala daemons per host, unless -Xmx is explicitly set. * If the heap size is limited to 1-2GB like below, then most tests pass but TestInsert.test_insert_large_string fails because IMPALA-4865 lets it create giant strings that eat up all the JVM heap. start-impala-cluster.py \ --impalad_args=--mem_limit_includes_jvm=true --jvm_args="-Xmx1g" Testing: Add a custom cluster test that uses the new option and validates the the memory consumption values. Change-Id: I39dd715882a32fc986755d573bd46f0fd9eefbfc Reviewed-on: http://gerrit.cloudera.org:8080/10928 Reviewed-by: Impala Public Jenkins Tested-by: Impala Public Jenkins --- M be/src/common/global-flags.cc M be/src/runtime/exec-env.cc M be/src/scheduling/admission-controller.cc M be/src/scheduling/query-schedule.h M be/src/scheduling/scheduler.cc M be/src/service/impala-http-handler.cc M be/src/service/impala-server.cc M be/src/util/memory-metrics.cc M be/src/util/memory-metrics.h M common/thrift/StatestoreService.thrift M tests/common/custom_cluster_test_suite.py M tests/custom_cluster/test_admission_controller.py A tests/custom_cluster/test_jvm_mem_tracking.py M www/backends.tmpl 14 files changed, 201 insertions(+), 58 deletions(-) Approvals: Impala Public Jenkins: Looks good to me, approved; Verified -- To view, visit http://gerrit.cloudera.org:8080/10928 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: I39dd715882a32fc986755d573bd46f0fd9eefbfc Gerrit-Change-Number: 10928 Gerrit-PatchSet: 16 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Pooja Nilangekar Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-7811: optionally count JVM heap towards process mem limit
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/10928 ) Change subject: IMPALA-7811: optionally count JVM heap towards process mem limit .. Patch Set 14: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/1529/ : 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/10928 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I39dd715882a32fc986755d573bd46f0fd9eefbfc Gerrit-Change-Number: 10928 Gerrit-PatchSet: 14 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Pooja Nilangekar Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Tue, 04 Dec 2018 04:42:46 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7811: optionally count JVM heap towards process mem limit
Hello Pooja Nilangekar, Bikramjeet Vig, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/10928 to look at the new patch set (#14). Change subject: IMPALA-7811: optionally count JVM heap towards process mem limit .. IMPALA-7811: optionally count JVM heap towards process mem limit Adds a flag --mem_limit_includes_jvm that alters memory accounting to include the amount of memory we think that the JVM is likely to use. By default this flag is false, so behaviour is unchanged. We're not ready to change the default but I want to check this in to enable experimentation. Two metrics are counted towards the process limit: * The maximum JVM heap size. We count this because the JVM memory consumption can expand up to this threshold at any time. * JVM non-heap committed memory. This can be a non-trivial amount of memory (e.g. I saw 150MB on one production cluster). There isn't a hard upper bound on this memory that I know of but should not grow rapidly. This requires adjustments in a couple of other places: * Admission control previous assumed that all of the process memory limit was available to queries (an assumption that is not strictly true because of untracked memory, etc, but close enough). However, the JVM heap makes a large part of the process limit unusable to queries, so we should only admit up to "process limit - max JVM heap size" per node. * The buffer pool is now a percentage of the remaining process limit after the JVM heap, instead of the total process limit. Currently, end-to-end tests fail if run with this flag for two reasons: * The default JVM heap size is 1/4 of physical memory, which means that essentially all of the process memory limit is consumed by the JVM heaps when we running 3 impala daemons per host, unless -Xmx is explicitly set. * If the heap size is limited to 1-2GB like below, then most tests pass but TestInsert.test_insert_large_string fails because IMPALA-4865 lets it create giant strings that eat up all the JVM heap. start-impala-cluster.py \ --impalad_args=--mem_limit_includes_jvm=true --jvm_args="-Xmx1g" Testing: Add a custom cluster test that uses the new option and validates the the memory consumption values. Change-Id: I39dd715882a32fc986755d573bd46f0fd9eefbfc --- M be/src/common/global-flags.cc M be/src/runtime/exec-env.cc M be/src/scheduling/admission-controller.cc M be/src/scheduling/query-schedule.h M be/src/scheduling/scheduler.cc M be/src/service/impala-http-handler.cc M be/src/service/impala-server.cc M be/src/util/memory-metrics.cc M be/src/util/memory-metrics.h M common/thrift/StatestoreService.thrift M tests/common/custom_cluster_test_suite.py M tests/custom_cluster/test_admission_controller.py A tests/custom_cluster/test_jvm_mem_tracking.py M www/backends.tmpl 14 files changed, 201 insertions(+), 58 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/28/10928/14 -- To view, visit http://gerrit.cloudera.org:8080/10928 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I39dd715882a32fc986755d573bd46f0fd9eefbfc Gerrit-Change-Number: 10928 Gerrit-PatchSet: 14 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Pooja Nilangekar Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-7811: optionally count JVM heap towards process mem limit
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/10928 ) Change subject: IMPALA-7811: optionally count JVM heap towards process mem limit .. Patch Set 15: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/10928 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I39dd715882a32fc986755d573bd46f0fd9eefbfc Gerrit-Change-Number: 10928 Gerrit-PatchSet: 15 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Pooja Nilangekar Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Tue, 04 Dec 2018 03:50:17 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7811: optionally count JVM heap towards process mem limit
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/10928 ) Change subject: IMPALA-7811: optionally count JVM heap towards process mem limit .. Patch Set 15: Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/3521/ DRY_RUN=false -- To view, visit http://gerrit.cloudera.org:8080/10928 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I39dd715882a32fc986755d573bd46f0fd9eefbfc Gerrit-Change-Number: 10928 Gerrit-PatchSet: 15 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Pooja Nilangekar Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Tue, 04 Dec 2018 03:50:18 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7811: optionally count JVM heap towards process mem limit
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/10928 ) Change subject: IMPALA-7811: optionally count JVM heap towards process mem limit .. Patch Set 14: Code-Review+2 (2 comments) http://gerrit.cloudera.org:8080/#/c/10928/13//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/10928/13//COMMIT_MSG@29 PS13, Line 29: > nit: long line Done http://gerrit.cloudera.org:8080/#/c/10928/13/tests/custom_cluster/test_jvm_mem_tracking.py File tests/custom_cluster/test_jvm_mem_tracking.py: http://gerrit.cloudera.org:8080/#/c/10928/13/tests/custom_cluster/test_jvm_mem_tracking.py@66 PS13, Line 66: * > (feel free to ignore this comment) maybe use 100 * MB here too since thats Done -- To view, visit http://gerrit.cloudera.org:8080/10928 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I39dd715882a32fc986755d573bd46f0fd9eefbfc Gerrit-Change-Number: 10928 Gerrit-PatchSet: 14 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Pooja Nilangekar Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Tue, 04 Dec 2018 03:49:50 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7811: optionally count JVM heap towards process mem limit
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/10928 ) Change subject: IMPALA-7811: optionally count JVM heap towards process mem limit .. Patch Set 13: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/1523/ : 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/10928 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I39dd715882a32fc986755d573bd46f0fd9eefbfc Gerrit-Change-Number: 10928 Gerrit-PatchSet: 13 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Pooja Nilangekar Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Tue, 04 Dec 2018 01:44:38 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7811: optionally count JVM heap towards process mem limit
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/10928 ) Change subject: IMPALA-7811: optionally count JVM heap towards process mem limit .. Patch Set 11: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/1522/ : 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/10928 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I39dd715882a32fc986755d573bd46f0fd9eefbfc Gerrit-Change-Number: 10928 Gerrit-PatchSet: 11 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Pooja Nilangekar Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Tue, 04 Dec 2018 01:33:13 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7811: optionally count JVM heap towards process mem limit
Hello Pooja Nilangekar, Bikramjeet Vig, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/10928 to look at the new patch set (#13). Change subject: IMPALA-7811: optionally count JVM heap towards process mem limit .. IMPALA-7811: optionally count JVM heap towards process mem limit Adds a flag --mem_limit_includes_jvm that alters memory accounting to include the amount of memory we think that the JVM is likely to use. By default this flag is false, so behaviour is unchanged. We're not ready to change the default but I want to check this in to enable experimentation. Two metrics are counted towards the process limit: * The maximum JVM heap size. We count this because the JVM memory consumption can expand up to this threshold at any time. * JVM non-heap committed memory. This can be a non-trivial amount of memory (e.g. I saw 150MB on one production cluster). There isn't a hard upper bound on this memory that I know of but should not grow rapidly. This requires adjustments in a couple of other places: * Admission control previous assumed that all of the process memory limit was available to queries (an assumption that is not strictly true because of untracked memory, etc, but close enough). However, the JVM heap makes a large part of the process limit unusable to queries, so we should only admit up to "process limit - max JVM heap size" per node. * The buffer pool is now a percentage of the remaining process limit after the JVM heap, instead of the total process limit. Currently, end-to-end tests fail if run with this flag for two reasons: * The default JVM heap size is 1/4 of physical memory, which means that essentially all of the process memory limit is consumed by the JVM heaps when we running 3 impala daemons per host, unless -Xmx is explicitly set. * If the heap size is limited to 1-2GB like below, then most tests pass but TestInsert.test_insert_large_string fails because IMPALA-4865 lets it create giant strings that eat up all the JVM heap. start-impala-cluster.py \ --impalad_args=--mem_limit_includes_jvm=true --jvm_args="-Xmx1g" Testing: Add a custom cluster test that uses the new option and validates the the memory consumption values. Change-Id: I39dd715882a32fc986755d573bd46f0fd9eefbfc --- M be/src/common/global-flags.cc M be/src/runtime/exec-env.cc M be/src/scheduling/admission-controller.cc M be/src/scheduling/query-schedule.h M be/src/scheduling/scheduler.cc M be/src/service/impala-http-handler.cc M be/src/service/impala-server.cc M be/src/util/memory-metrics.cc M be/src/util/memory-metrics.h M common/thrift/StatestoreService.thrift M tests/common/custom_cluster_test_suite.py M tests/custom_cluster/test_admission_controller.py A tests/custom_cluster/test_jvm_mem_tracking.py M www/backends.tmpl 14 files changed, 201 insertions(+), 58 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/28/10928/13 -- To view, visit http://gerrit.cloudera.org:8080/10928 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I39dd715882a32fc986755d573bd46f0fd9eefbfc Gerrit-Change-Number: 10928 Gerrit-PatchSet: 13 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Pooja Nilangekar Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-7811: optionally count JVM heap towards process mem limit
Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/10928 ) Change subject: IMPALA-7811: optionally count JVM heap towards process mem limit .. Patch Set 13: Code-Review+2 (3 comments) http://gerrit.cloudera.org:8080/#/c/10928/13//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/10928/13//COMMIT_MSG@29 PS13, Line 29: size" nit: long line http://gerrit.cloudera.org:8080/#/c/10928/10/be/src/util/memory-metrics.cc File be/src/util/memory-metrics.cc: http://gerrit.cloudera.org:8080/#/c/10928/10/be/src/util/memory-metrics.cc@196 PS10, Line 196: HEAP_MAX_USAGE > Documented the metric names (I think using the public name is more intuitiv works for me, i was just a bit concerned since those names are populated manually in code rather than fetching it from a standard java enum/class http://gerrit.cloudera.org:8080/#/c/10928/13/tests/custom_cluster/test_jvm_mem_tracking.py File tests/custom_cluster/test_jvm_mem_tracking.py: http://gerrit.cloudera.org:8080/#/c/10928/13/tests/custom_cluster/test_jvm_mem_tracking.py@66 PS13, Line 66: e6 (feel free to ignore this comment) maybe use 100 * MB here too since thats what is mentioned in the comment above -- To view, visit http://gerrit.cloudera.org:8080/10928 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I39dd715882a32fc986755d573bd46f0fd9eefbfc Gerrit-Change-Number: 10928 Gerrit-PatchSet: 13 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Pooja Nilangekar Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Tue, 04 Dec 2018 00:51:37 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7811: optionally count JVM heap towards process mem limit
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/10928 ) Change subject: IMPALA-7811: optionally count JVM heap towards process mem limit .. Patch Set 11: (1 comment) http://gerrit.cloudera.org:8080/#/c/10928/11/tests/custom_cluster/test_admission_controller.py File tests/custom_cluster/test_admission_controller.py: http://gerrit.cloudera.org:8080/#/c/10928/11/tests/custom_cluster/test_admission_controller.py@520 PS11, Line 520: t > flake8: E501 line too long (95 > 90 characters) Done -- To view, visit http://gerrit.cloudera.org:8080/10928 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I39dd715882a32fc986755d573bd46f0fd9eefbfc Gerrit-Change-Number: 10928 Gerrit-PatchSet: 11 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Pooja Nilangekar Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Tue, 04 Dec 2018 00:45:49 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7811: optionally count JVM heap towards process mem limit
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/10928 ) Change subject: IMPALA-7811: optionally count JVM heap towards process mem limit .. Patch Set 11: (1 comment) http://gerrit.cloudera.org:8080/#/c/10928/11/tests/custom_cluster/test_admission_controller.py File tests/custom_cluster/test_admission_controller.py: http://gerrit.cloudera.org:8080/#/c/10928/11/tests/custom_cluster/test_admission_controller.py@520 PS11, Line 520: t flake8: E501 line too long (95 > 90 characters) -- To view, visit http://gerrit.cloudera.org:8080/10928 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I39dd715882a32fc986755d573bd46f0fd9eefbfc Gerrit-Change-Number: 10928 Gerrit-PatchSet: 11 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Pooja Nilangekar Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Tue, 04 Dec 2018 00:44:17 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7811: optionally count JVM heap towards process mem limit
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/10928 ) Change subject: IMPALA-7811: optionally count JVM heap towards process mem limit .. Patch Set 10: (5 comments) http://gerrit.cloudera.org:8080/#/c/10928/8/be/src/runtime/exec-env.cc File be/src/runtime/exec-env.cc: http://gerrit.cloudera.org:8080/#/c/10928/8/be/src/runtime/exec-env.cc@277 PS8, Line 277: // The JVM max heap size is static and therefore known at this point > In that case would it make sense to always account for "JvmMemoryMetric::NO The maximum amount of non-heap memory isn't fixed though is the problem. On /metrics on my system: jvm.non-heap.max-usage-bytes -1.00 B Jvm non-heap Max Usage Bytes http://gerrit.cloudera.org:8080/#/c/10928/10/be/src/scheduling/query-schedule.h File be/src/scheduling/query-schedule.h: http://gerrit.cloudera.org:8080/#/c/10928/10/be/src/scheduling/query-schedule.h@75 PS10, Line 75: should > nit: can Done http://gerrit.cloudera.org:8080/#/c/10928/10/be/src/util/memory-metrics.cc File be/src/util/memory-metrics.cc: http://gerrit.cloudera.org:8080/#/c/10928/10/be/src/util/memory-metrics.cc@196 PS10, Line 196: HEAP_MAX_USAGE > is it worth documenting in the header file that these correspond to the "he Documented the metric names (I think using the public name is more intuitive than referencing another part of the code). http://gerrit.cloudera.org:8080/#/c/10928/10/common/thrift/StatestoreService.thrift File common/thrift/StatestoreService.thrift: http://gerrit.cloudera.org:8080/#/c/10928/10/common/thrift/StatestoreService.thrift@76 PS10, Line 76: admit_mem_limit > we might have to change the wording on AdmissionController::REASON_REQ_OVER Done http://gerrit.cloudera.org:8080/#/c/10928/10/tests/custom_cluster/test_jvm_mem_tracking.py File tests/custom_cluster/test_jvm_mem_tracking.py: http://gerrit.cloudera.org:8080/#/c/10928/10/tests/custom_cluster/test_jvm_mem_tracking.py@50 PS10, Line 50: GB > nit: MB? doh -- To view, visit http://gerrit.cloudera.org:8080/10928 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I39dd715882a32fc986755d573bd46f0fd9eefbfc Gerrit-Change-Number: 10928 Gerrit-PatchSet: 10 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Pooja Nilangekar Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Tue, 04 Dec 2018 00:43:19 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7811: optionally count JVM heap towards process mem limit
Hello Pooja Nilangekar, Bikramjeet Vig, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/10928 to look at the new patch set (#11). Change subject: IMPALA-7811: optionally count JVM heap towards process mem limit .. IMPALA-7811: optionally count JVM heap towards process mem limit Adds a flag --mem_limit_includes_jvm that alters memory accounting to include the amount of memory we think that the JVM is likely to use. By default this flag is false, so behaviour is unchanged. We're not ready to change the default but I want to check this in to enable experimentation. Two metrics are counted towards the process limit: * The maximum JVM heap size. We count this because the JVM memory consumption can expand up to this threshold at any time. * JVM non-heap committed memory. This can be a non-trivial amount of memory (e.g. I saw 150MB on one production cluster). There isn't a hard upper bound on this memory that I know of but should not grow rapidly. This requires adjustments in a couple of other places: * Admission control previous assumed that all of the process memory limit was available to queries (an assumption that is not strictly true because of untracked memory, etc, but close enough). However, the JVM heap makes a large part of the process limit unusable to queries, so we should only admit up to "process limit - max JVM heap size" per node. * The buffer pool is now a percentage of the remaining process limit after the JVM heap, instead of the total process limit. Currently, end-to-end tests fail if run with this flag for two reasons: * The default JVM heap size is 1/4 of physical memory, which means that essentially all of the process memory limit is consumed by the JVM heaps when we running 3 impala daemons per host, unless -Xmx is explicitly set. * If the heap size is limited to 1-2GB like below, then most tests pass but TestInsert.test_insert_large_string fails because IMPALA-4865 lets it create giant strings that eat up all the JVM heap. start-impala-cluster.py \ --impalad_args=--mem_limit_includes_jvm=true --jvm_args="-Xmx1g" Testing: Add a custom cluster test that uses the new option and validates the the memory consumption values. Change-Id: I39dd715882a32fc986755d573bd46f0fd9eefbfc --- M be/src/common/global-flags.cc M be/src/runtime/exec-env.cc M be/src/scheduling/admission-controller.cc M be/src/scheduling/query-schedule.h M be/src/scheduling/scheduler.cc M be/src/service/impala-http-handler.cc M be/src/service/impala-server.cc M be/src/util/memory-metrics.cc M be/src/util/memory-metrics.h M common/thrift/StatestoreService.thrift M tests/common/custom_cluster_test_suite.py M tests/custom_cluster/test_admission_controller.py A tests/custom_cluster/test_jvm_mem_tracking.py M www/backends.tmpl 14 files changed, 200 insertions(+), 58 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/28/10928/11 -- To view, visit http://gerrit.cloudera.org:8080/10928 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I39dd715882a32fc986755d573bd46f0fd9eefbfc Gerrit-Change-Number: 10928 Gerrit-PatchSet: 11 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Pooja Nilangekar Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-7811: optionally count JVM heap towards process mem limit
Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/10928 ) Change subject: IMPALA-7811: optionally count JVM heap towards process mem limit .. Patch Set 10: (4 comments) http://gerrit.cloudera.org:8080/#/c/10928/10/be/src/scheduling/query-schedule.h File be/src/scheduling/query-schedule.h: http://gerrit.cloudera.org:8080/#/c/10928/10/be/src/scheduling/query-schedule.h@75 PS10, Line 75: should nit: can http://gerrit.cloudera.org:8080/#/c/10928/10/be/src/util/memory-metrics.cc File be/src/util/memory-metrics.cc: http://gerrit.cloudera.org:8080/#/c/10928/10/be/src/util/memory-metrics.cc@196 PS10, Line 196: HEAP_MAX_USAGE is it worth documenting in the header file that these correspond to the "heap" and "non-heap" pool names as populated by JniUtil.getJvmMemoryMetrics() http://gerrit.cloudera.org:8080/#/c/10928/10/common/thrift/StatestoreService.thrift File common/thrift/StatestoreService.thrift: http://gerrit.cloudera.org:8080/#/c/10928/10/common/thrift/StatestoreService.thrift@76 PS10, Line 76: admit_mem_limit we might have to change the wording on AdmissionController::REASON_REQ_OVER_NODE_MEM error message, since it refers to the process mem_limit http://gerrit.cloudera.org:8080/#/c/10928/10/tests/custom_cluster/test_jvm_mem_tracking.py File tests/custom_cluster/test_jvm_mem_tracking.py: http://gerrit.cloudera.org:8080/#/c/10928/10/tests/custom_cluster/test_jvm_mem_tracking.py@50 PS10, Line 50: GB nit: MB? -- To view, visit http://gerrit.cloudera.org:8080/10928 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I39dd715882a32fc986755d573bd46f0fd9eefbfc Gerrit-Change-Number: 10928 Gerrit-PatchSet: 10 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Pooja Nilangekar Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Mon, 03 Dec 2018 23:57:07 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7811: optionally count JVM heap towards process mem limit
Pooja Nilangekar has posted comments on this change. ( http://gerrit.cloudera.org:8080/10928 ) Change subject: IMPALA-7811: optionally count JVM heap towards process mem limit .. Patch Set 10: Code-Review+1 (1 comment) Looks good. http://gerrit.cloudera.org:8080/#/c/10928/8/be/src/runtime/exec-env.cc File be/src/runtime/exec-env.cc: http://gerrit.cloudera.org:8080/#/c/10928/8/be/src/runtime/exec-env.cc@277 PS8, Line 277: // The JVM max heap size is static and therefore known at this point > I chose not to do that since it's dynamic and unpredictable - we would end In that case would it make sense to always account for "JvmMemoryMetric::NON_HEAP_MAX". I believe that value is fixed when a JVM starts up. That way, we can always cap it at the upper bound, similar to the heap memory. -- To view, visit http://gerrit.cloudera.org:8080/10928 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I39dd715882a32fc986755d573bd46f0fd9eefbfc Gerrit-Change-Number: 10928 Gerrit-PatchSet: 10 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Pooja Nilangekar Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Mon, 03 Dec 2018 23:52:24 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7811: optionally count JVM heap towards process mem limit
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/10928 ) Change subject: IMPALA-7811: optionally count JVM heap towards process mem limit .. Patch Set 9: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/1440/ : 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/10928 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I39dd715882a32fc986755d573bd46f0fd9eefbfc Gerrit-Change-Number: 10928 Gerrit-PatchSet: 9 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Pooja Nilangekar Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Tue, 27 Nov 2018 02:24:19 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7811: optionally count JVM heap towards process mem limit
Hello Pooja Nilangekar, Bikramjeet Vig, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/10928 to look at the new patch set (#9). Change subject: IMPALA-7811: optionally count JVM heap towards process mem limit .. IMPALA-7811: optionally count JVM heap towards process mem limit Adds a flag --mem_limit_includes_jvm that alters memory accounting to include the amount of memory we think that the JVM is likely to use. By default this flag is false, so behaviour is unchanged. We're not ready to change the default but I want to check this in to enable experimentation. Two metrics are counted towards the process limit: * The maximum JVM heap size. We count this because the JVM memory consumption can expand up to this threshold at any time. * JVM non-heap committed memory. This can be a non-trivial amount of memory (e.g. I saw 150MB on one production cluster). There isn't a hard upper bound on this memory that I know of but should not grow rapidly. This requires adjustments in a couple of other places: * Admission control previous assumed that all of the process memory limit was available to queries (an assumption that is not strictly true because of untracked memory, etc, but close enough). However, the JVM heap makes a large part of the process limit unusable to queries, so we should only admit up to "process limit - max JVM heap size" per node. * The buffer pool is now a percentage of the remaining process limit after the JVM heap, instead of the total process limit. Currently, end-to-end tests fail if run with this flag for two reasons: * The default JVM heap size is 1/4 of physical memory, which means that essentially all of the process memory limit is consumed by the JVM heaps when we running 3 impala daemons per host, unless -Xmx is explicitly set. * If the heap size is limited to 1-2GB like below, then most tests pass but TestInsert.test_insert_large_string fails because IMPALA-4865 lets it create giant strings that eat up all the JVM heap. start-impala-cluster.py \ --impalad_args=--mem_limit_includes_jvm=true --jvm_args="-Xmx1g" Testing: Add a custom cluster test that uses the new option and validates the the memory consumption values. Change-Id: I39dd715882a32fc986755d573bd46f0fd9eefbfc --- M be/src/common/global-flags.cc M be/src/runtime/exec-env.cc M be/src/scheduling/admission-controller.cc M be/src/scheduling/query-schedule.h M be/src/scheduling/scheduler.cc M be/src/service/impala-http-handler.cc M be/src/service/impala-server.cc M be/src/util/memory-metrics.cc M be/src/util/memory-metrics.h M common/thrift/StatestoreService.thrift M tests/common/custom_cluster_test_suite.py A tests/custom_cluster/test_jvm_mem_tracking.py M www/backends.tmpl 13 files changed, 192 insertions(+), 55 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/28/10928/9 -- To view, visit http://gerrit.cloudera.org:8080/10928 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I39dd715882a32fc986755d573bd46f0fd9eefbfc Gerrit-Change-Number: 10928 Gerrit-PatchSet: 9 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Pooja Nilangekar Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-7811: optionally count JVM heap towards process mem limit
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/10928 ) Change subject: IMPALA-7811: optionally count JVM heap towards process mem limit .. Patch Set 8: (5 comments) http://gerrit.cloudera.org:8080/#/c/10928/8//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/10928/8//COMMIT_MSG@22 PS8, Line 22: row > nit: typo Done http://gerrit.cloudera.org:8080/#/c/10928/8//COMMIT_MSG@25 PS8, Line 25: We should not admit query memory in addition to the JVM memory. > Initially, I had some trouble understanding this. Could you please clarify Done http://gerrit.cloudera.org:8080/#/c/10928/8/be/src/runtime/exec-env.cc File be/src/runtime/exec-env.cc: http://gerrit.cloudera.org:8080/#/c/10928/8/be/src/runtime/exec-env.cc@277 PS8, Line 277: post_jvm_bytes_limit -= JvmMemoryMetric::HEAP_MAX_USAGE->GetValue(); > Is there a reason for not subtracting the "JvmMemoryMetric::NON_HEAP_COMMIT I chose not to do that since it's dynamic and unpredictable - we would end up with the buffer pool size potentially varying from run-to-run which would be weird. http://gerrit.cloudera.org:8080/#/c/10928/8/be/src/service/impala-server.cc File be/src/service/impala-server.cc: http://gerrit.cloudera.org:8080/#/c/10928/8/be/src/service/impala-server.cc@1817 PS8, Line 1817: admit_mem_limit -= JvmMemoryMetric::HEAP_MAX_USAGE->GetValue(); > Same concern as above. Because it's dynamic. It would also be a bit weird since we would then in theory want to send an update to the statestore every time that the JVM memory consumption change a little bit. http://gerrit.cloudera.org:8080/#/c/10928/8/be/src/service/impala-server.cc@1817 PS8, Line 1817: ; > nit: extra semicolon Done -- To view, visit http://gerrit.cloudera.org:8080/10928 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I39dd715882a32fc986755d573bd46f0fd9eefbfc Gerrit-Change-Number: 10928 Gerrit-PatchSet: 8 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Pooja Nilangekar Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Tue, 27 Nov 2018 01:47:01 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7811: optionally count JVM heap towards process mem limit
Pooja Nilangekar has posted comments on this change. ( http://gerrit.cloudera.org:8080/10928 ) Change subject: IMPALA-7811: optionally count JVM heap towards process mem limit .. Patch Set 8: (5 comments) The approach used to compute the memory utilized and accounting for it in the process memory makes sense. Thanks for adding the test to validate it. http://gerrit.cloudera.org:8080/#/c/10928/8//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/10928/8//COMMIT_MSG@22 PS8, Line 22: row nit: typo http://gerrit.cloudera.org:8080/#/c/10928/8//COMMIT_MSG@25 PS8, Line 25: We should not admit query memory in addition to the JVM memory. Initially, I had some trouble understanding this. Could you please clarify this? http://gerrit.cloudera.org:8080/#/c/10928/8/be/src/runtime/exec-env.cc File be/src/runtime/exec-env.cc: http://gerrit.cloudera.org:8080/#/c/10928/8/be/src/runtime/exec-env.cc@277 PS8, Line 277: post_jvm_bytes_limit -= JvmMemoryMetric::HEAP_MAX_USAGE->GetValue(); Is there a reason for not subtracting the "JvmMemoryMetric::NON_HEAP_COMMITTED" memory from this value? http://gerrit.cloudera.org:8080/#/c/10928/8/be/src/service/impala-server.cc File be/src/service/impala-server.cc: http://gerrit.cloudera.org:8080/#/c/10928/8/be/src/service/impala-server.cc@1817 PS8, Line 1817: ; nit: extra semicolon http://gerrit.cloudera.org:8080/#/c/10928/8/be/src/service/impala-server.cc@1817 PS8, Line 1817: admit_mem_limit -= JvmMemoryMetric::HEAP_MAX_USAGE->GetValue(); Same concern as above. Is there a reason for not subtracting the "JvmMemoryMetric::NON_HEAP_COMMITTED" memory from this value? -- To view, visit http://gerrit.cloudera.org:8080/10928 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I39dd715882a32fc986755d573bd46f0fd9eefbfc Gerrit-Change-Number: 10928 Gerrit-PatchSet: 8 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Pooja Nilangekar Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Wed, 21 Nov 2018 01:15:42 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7811: optionally count JVM heap towards process mem limit
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/10928 ) Change subject: IMPALA-7811: optionally count JVM heap towards process mem limit .. Patch Set 8: Hey Pooja, Bikram won't be able to review this for a couple of weeks but maybe this will be interesting for you to look at. I can talk through the details of the memory accounting in person if that would be helpful (it would be good to understand in any case) -- To view, visit http://gerrit.cloudera.org:8080/10928 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I39dd715882a32fc986755d573bd46f0fd9eefbfc Gerrit-Change-Number: 10928 Gerrit-PatchSet: 8 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Pooja Nilangekar Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Fri, 16 Nov 2018 22:22:15 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7811: optionally count JVM heap towards process mem limit
Tim Armstrong has uploaded a new patch set (#8). ( http://gerrit.cloudera.org:8080/10928 ) Change subject: IMPALA-7811: optionally count JVM heap towards process mem limit .. IMPALA-7811: optionally count JVM heap towards process mem limit Adds a flag --mem_limit_includes_jvm that alters memory accounting to include the amount of memory we think that the JVM is likely to use. By default this flag is false, so behaviour is unchanged. We're not ready to change the default but I want to check this in to enable experimentation. Two metrics are counted towards the process limit: * The maximum JVM heap size. We count this because the JVM memory consumption can expand up to this threshold at any time. * JVM non-heap committed memory. This can be a non-trivial amount of memory (e.g. I saw 150MB on one production cluster). There isn't a hard upper bound on this memory that I know of but should not row rapidly. This requires adjustments in a couple of other places: * We should not admit query memory in addition to the JVM memory. * The buffer pool is now a percentage of the remaining process limit after the JVM heap, instead of the total process limit. Currently, end-to-end tests fail if run with this flag for two reasons: * The default JVM heap size is 1/4 of physical memory, which means that essentially all of the process memory limit is consumed by the JVM heaps when we running 3 impala daemons per host, unless -Xmx is explicitly set. * If the heap size is limited to 1-2GB like below, then most tests pass but TestInsert.test_insert_large_string fails because IMPALA-4865 lets it create giant strings that eat up all the JVM heap. start-impala-cluster.py \ --impalad_args=--mem_limit_includes_jvm=true --jvm_args="-Xmx1g" Testing: Add a custom cluster test that uses the new option and validates the the memory consumption values. Change-Id: I39dd715882a32fc986755d573bd46f0fd9eefbfc --- M be/src/common/global-flags.cc M be/src/runtime/exec-env.cc M be/src/scheduling/admission-controller.cc M be/src/scheduling/query-schedule.h M be/src/scheduling/scheduler.cc M be/src/service/impala-http-handler.cc M be/src/service/impala-server.cc M be/src/util/memory-metrics.cc M be/src/util/memory-metrics.h M common/thrift/StatestoreService.thrift M tests/common/custom_cluster_test_suite.py A tests/custom_cluster/test_jvm_mem_tracking.py M www/backends.tmpl 13 files changed, 187 insertions(+), 55 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/28/10928/8 -- To view, visit http://gerrit.cloudera.org:8080/10928 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I39dd715882a32fc986755d573bd46f0fd9eefbfc Gerrit-Change-Number: 10928 Gerrit-PatchSet: 8 Gerrit-Owner: Tim Armstrong
