Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/11157 )
Change subject: IMPALA-7349: Add Admission control support for automatically setting mem_limit ...................................................................... Patch Set 7: (5 comments) http://gerrit.cloudera.org:8080/#/c/11157/7//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/11157/7//COMMIT_MSG@10 PS7, Line 10: With this patch the mem_limit of a query is automatically set based on : the mem_limit set in the query options and the mem_estimate calculated : by the planner. > could you go into more detail what this means? how is the "output" mem_limi For the new behavior (Assuming either min or max memlimit is set in the pool): if mem_limit is set in query options: use that and if 'strict-min-max-query-mem-limit' is true, enforce the min/max limits defined in the pool config. else: mem_limit = max(mem_estiamte, min_mem_limit_required_to_accomodate_largest_min_reservation) and enforce min/max limits defined in the pool config on this value. http://gerrit.cloudera.org:8080/#/c/11157/7//COMMIT_MSG@16 PS7, Line 16: If these are set to zero > is it if any or all are set to zero? Both need to be set to zero http://gerrit.cloudera.org:8080/#/c/11157/7//COMMIT_MSG@18 PS7, Line 18: if false, the mem_limit defined in : the query options is used directly and the max/min limits are not : enforced on it. > does that mean this option effectively disables this new behavior entirely? this only disables a part of it for the case when mem_limit is defined in the query options. This was added as a workaround for the case where a query cannot run if the min/max limits are applied to it http://gerrit.cloudera.org:8080/#/c/11157/7/common/thrift/ImpalaInternalService.thrift File common/thrift/ImpalaInternalService.thrift: http://gerrit.cloudera.org:8080/#/c/11157/7/common/thrift/ImpalaInternalService.thrift@783 PS7, Line 783: // If TRUE, the mem_limit query option will not be bounded by the max/min query mem : // limits specified for the pool. Default is FALSE. need to update this as with the change in name it would mean the opposite http://gerrit.cloudera.org:8080/#/c/11157/7/common/thrift/metrics.json File common/thrift/metrics.json: http://gerrit.cloudera.org:8080/#/c/11157/7/common/thrift/metrics.json@53 PS7, Line 53: "description": "Resource Pool $0 Max Query Memory Limit", > did you sanity check that all these metrics look right on the metrics page? Yes, I manually checked them. also one of them, namely admission-controller.pool-max-query-mem-limit is used in one of the tests that dynamically changes the pool config -- To view, visit http://gerrit.cloudera.org:8080/11157 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ifec00141651982f5975803c2165b7d7a10ebeaa6 Gerrit-Change-Number: 11157 Gerrit-PatchSet: 7 Gerrit-Owner: Bikramjeet Vig <bikramjeet....@cloudera.com> Gerrit-Reviewer: Bikramjeet Vig <bikramjeet....@cloudera.com> Gerrit-Reviewer: Dan Hecht <dhe...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Comment-Date: Wed, 22 Aug 2018 00:28:19 +0000 Gerrit-HasComments: Yes
