Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/10058 )
Change subject: IMPALA-6847: work around high memory estimates for AC ...................................................................... Patch Set 12: (3 comments) +1 for the simplified version (patch 12). It is way cleaner and also looks more like a workaround, which is what is intended. However, I am a bit concerned about this option becoming mainstream, since this is a workaround and will probably be deprecated in the future. We should discourage use of this unless absolutely necessary and ask users to use it at a more granular level (pool level) so that it will be easily manageable and removable once deprecated. I also really like the ideas in patch 8 and looks like a good direction to steer our solution towards in IMPALA-6460. It was essentially decoupling the 2 functions performed by mem_limit ( 1. to enforce a runtime limit and 2. to be used in admission control for admitting and accounting of memory) and would have made impala more stable, but the fact that a query option was used to override mem_limit's functionality and had overlapping functionality with mem_estimate (acting as an upper limit for it) was making it confusing. http://gerrit.cloudera.org:8080/#/c/10058/12/fe/src/main/java/org/apache/impala/service/Frontend.java File fe/src/main/java/org/apache/impala/service/Frontend.java: http://gerrit.cloudera.org:8080/#/c/10058/12/fe/src/main/java/org/apache/impala/service/Frontend.java@1063 PS12, Line 1063: set typo: sent http://gerrit.cloudera.org:8080/#/c/10058/12/fe/src/main/java/org/apache/impala/service/Frontend.java@1129 PS12, Line 1129: checkForMemEstimateOverride nit: how about checkAndOverrideMemEstimate http://gerrit.cloudera.org:8080/#/c/10058/12/testdata/workloads/functional-query/queries/QueryTest/admission-reject-mem-estimate.test File testdata/workloads/functional-query/queries/QueryTest/admission-reject-mem-estimate.test: http://gerrit.cloudera.org:8080/#/c/10058/12/testdata/workloads/functional-query/queries/QueryTest/admission-reject-mem-estimate.test@1 PS12, Line 1: ==== add case where mem_limit and max_mem_estimate_for_admission both are set, to show that mem_limit is used over max_mem_estimate_for_admission -- To view, visit http://gerrit.cloudera.org:8080/10058 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ia5fc32a507ad0f00f564dfe4f954a829ac55d14e Gerrit-Change-Number: 10058 Gerrit-PatchSet: 12 Gerrit-Owner: Tim Armstrong <[email protected]> Gerrit-Reviewer: Bikramjeet Vig <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Philip Zeyliger <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Comment-Date: Tue, 17 Apr 2018 05:07:45 +0000 Gerrit-HasComments: Yes
