Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/11157 )
Change subject: IMPALA-7349: Add Admission control support for automatically setting per host memory limit for a query ...................................................................... Patch Set 7: (2 comments) http://gerrit.cloudera.org:8080/#/c/11157/8/be/src/runtime/test-env.cc File be/src/runtime/test-env.cc: http://gerrit.cloudera.org:8080/#/c/11157/8/be/src/runtime/test-env.cc@116 PS8, Line 116: int64_t mem_limit = query_options->__isset.mem_limit ? query_options->mem_limit : -1; formatting is a bit weird - is this what clang-format did? If so, ok to leave. http://gerrit.cloudera.org:8080/#/c/11157/7/be/src/scheduling/admission-controller.cc File be/src/scheduling/admission-controller.cc: http://gerrit.cloudera.org:8080/#/c/11157/7/be/src/scheduling/admission-controller.cc@1062 PS7, Line 1062: int64_t AdmissionController::ComputePerHostMemLimit( : const QuerySchedule& schedule, const TPoolConfig& pool_cfg, bool for_admission) { : // If the min_query_mem_limit and max_query_mem_limit are not set in the pool config : // then it falls back to traditional(old) behaviour, which means that, if for_admission : // is false, it returns the mem_limit if it is set in the query options, else returns -1 : // which means no limit; if for_admission is true, it returns the mem_limit if it is set : // in the query options, else returns the per host mem estimate calculated during : // planning. : bool mimic_old_behaviour = : pool_cfg.min_query_mem_limit == 0 && pool_cfg.max_query_mem_limit == 0; : : int64_t per_host_mem_limit = 0; : bool has_query_option = false; : const TQueryOptions& query_options = schedule.query_options(); : if (query_options.__isset.mem_limit && query_options.mem_limit > 0) { : per_host_mem_limit = query_options.mem_limit; : has_query_option = true; : } else if (!for_admission && mimic_old_behaviour) { : return -1; : } : : if (!has_query_option) { : per_host_mem_limit = schedule.GetPerHostMemoryEstimate(); : if (!mimic_old_behaviour) { : int64_t min_mem_limit_required = ReservationUtil::GetMinMemLimitFromReservation( : schedule.largest_min_reservation()); : per_host_mem_limit = max(per_host_mem_limit, min_mem_limit_required); : } : } : : if (!has_query_option || !pool_cfg.strict_min_max_query_mem_limit) { : DCHECK(pool_cfg.max_query_mem_limit <= 0 : || pool_cfg.max_query_mem_limit >= pool_cfg.min_query_mem_limit); : if (pool_cfg.min_query_mem_limit > 0) { : per_host_mem_limit = max(per_host_mem_limit, pool_cfg.min_query_mem_limit); : } : if (pool_cfg.max_query_mem_limit > 0) { : per_host_mem_limit = min(per_host_mem_limit, pool_cfg.max_query_mem_limit); : } : } : : return per_host_mem_limit; : } > how about I move this into QuerySchedule and do a schedule->UpdateMemREquir I think that's ok, should just make sure we document the invariants and usage patterns in QuerySchedule. -- 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 <[email protected]> Gerrit-Reviewer: Bikramjeet Vig <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Comment-Date: Sat, 15 Sep 2018 00:08:31 +0000 Gerrit-HasComments: Yes
