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 <tarmstr...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bikramjeet....@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhe...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <phi...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com>
Gerrit-Comment-Date: Tue, 17 Apr 2018 05:07:45 +0000
Gerrit-HasComments: Yes

Reply via email to