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 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. This mem_limit wil ...................................................................... Patch Set 6: (39 comments) http://gerrit.cloudera.org:8080/#/c/11157/6/be/src/runtime/mem-tracker.h File be/src/runtime/mem-tracker.h: http://gerrit.cloudera.org:8080/#/c/11157/6/be/src/runtime/mem-tracker.h@126 PS6, Line 126: const > Unnecessary const qualifier when passing by value Done http://gerrit.cloudera.org:8080/#/c/11157/6/be/src/runtime/query-exec-mgr.h File be/src/runtime/query-exec-mgr.h: http://gerrit.cloudera.org:8080/#/c/11157/6/be/src/runtime/query-exec-mgr.h@57 PS6, Line 57: const > Unnecessary const qualifier Done http://gerrit.cloudera.org:8080/#/c/11157/6/be/src/runtime/query-exec-mgr.h@75 PS6, Line 75: const > Unnecessary const qualifier Done http://gerrit.cloudera.org:8080/#/c/11157/6/be/src/runtime/query-state.h File be/src/runtime/query-state.h: http://gerrit.cloudera.org:8080/#/c/11157/6/be/src/runtime/query-state.h@251 PS6, Line 251: /// Create QueryState w/ refcnt of 0. > Maybe worth documenting that 'mem_limit' applies to the query MemTracker? I Done http://gerrit.cloudera.org:8080/#/c/11157/6/be/src/runtime/query-state.h@254 PS6, Line 254: const > Unnecessary const qualifier Done http://gerrit.cloudera.org:8080/#/c/11157/6/be/src/scheduling/admission-controller.h File be/src/scheduling/admission-controller.h: http://gerrit.cloudera.org:8080/#/c/11157/6/be/src/scheduling/admission-controller.h@89 PS6, Line 89: /// The memory required for admission for a request is specified as the query option > This comment needs update. more parts of this need to be updated. I ll update it when we decide on the right names for the new config parameters http://gerrit.cloudera.org:8080/#/c/11157/6/be/src/scheduling/admission-controller.h@188 PS6, Line 188: /// > Is it worth summarizing the configuration storing, i.e. what the config fil yup, sounds like that ll save everyone some time. I ll make the changes to the description later like i mentioned above. http://gerrit.cloudera.org:8080/#/c/11157/6/be/src/scheduling/admission-controller.h@191 PS6, Line 191: /// TODO: Remove less important debug logging after more cluster testing. Should have a > While we're here, maybe remove this TODO? Done http://gerrit.cloudera.org:8080/#/c/11157/6/be/src/scheduling/admission-controller.h@252 PS6, Line 252: limit_ > Not sure what variable this is referring to? Done http://gerrit.cloudera.org:8080/#/c/11157/6/be/src/scheduling/admission-controller.h@256 PS6, Line 256: /// The per host mem admitted only for the queries admitted locally. > Add a blank line between members with comments Done http://gerrit.cloudera.org:8080/#/c/11157/6/be/src/scheduling/admission-controller.h@260 PS6, Line 260: class PoolStats { > Is this a good chance to move this class to a separate file? Maybe as a fol Lets do that on a follow up, I would like to put more effort into what other functionality we can abstract away in a separate resource pool class. filed IMPALA-7454 http://gerrit.cloudera.org:8080/#/c/11157/6/be/src/scheduling/admission-controller.cc File be/src/scheduling/admission-controller.cc: http://gerrit.cloudera.org:8080/#/c/11157/6/be/src/scheduling/admission-controller.cc@93 PS6, Line 93: y-mem-limit > I'm not 100% on this mem_limit naming - it might be describing the current makes sense. I agree with your suggestion, but will defer the change till we get input from others and decide on a final name, since making this changes requires alot of renaming. http://gerrit.cloudera.org:8080/#/c/11157/6/be/src/scheduling/admission-controller.cc@115 PS6, Line 115: const string PROFILE_INFO_KEY_ADMITTED_MEM = "Memory Admitted"; > It's a little ambiguous based on the name whether this is per-host memory o Done http://gerrit.cloudera.org:8080/#/c/11157/6/be/src/scheduling/admission-controller.cc@351 PS6, Line 351: per_node_mem_needed > per_host_mem_limit for consistency? changed to per_host_mem_to_admit which conveys the exact use. Added DCHECK. http://gerrit.cloudera.org:8080/#/c/11157/6/be/src/scheduling/admission-controller.cc@391 PS6, Line 391: const TQueryOptions& query_opts = schedule.query_options(); > Should this be a separate function? It seems like it is a self-contained ca Done http://gerrit.cloudera.org:8080/#/c/11157/6/be/src/scheduling/admission-controller.cc@417 PS6, Line 417: over the mem limit > This is a little misleading since it's not necessarily a mem_limit. Maybe " Done http://gerrit.cloudera.org:8080/#/c/11157/6/be/src/scheduling/admission-controller.cc@483 PS6, Line 483: : // The per_host_mem_limit also caps the max initial reservation possible, so check to : // see if it is high enough to accommodate the largest min_reservation. Cases where : // these are possible: : // 1. The pool.max_query_mem_limit is set too low : // 2. mem_limit in query options is set low and no max/min_query_mem_limit is set in : // the pool config. : // 3. mem_limit in query options is set low and min_query_mem_limit is also set low. : // 4. mem_limit in query options is set low and the pool.min_query_mem_limit is set : // to a higher value but pool.strict_min_max_query_mem_limit is true. : const int64_t per_host_mem_limit = ComputePerHostMemLimit(schedule, pool_cfg, false); : if (per_host_mem_limit > 0) { : const int64_t max_reservation = : ReservationUtil::GetReservationLimitFromMemLimit(per_host_mem_limit); : if (largest_min_mem_reservation.second > max_reservation) { : const int64_t required_mem_limit = ReservationUtil::GetMinMemLimitFromReservation( : largest_min_mem_reservation.second); : *rejection_reason = Substitute(REASON_MEM_LIMIT_TOO_LOW_FOR_RESERVATION, : PrintBytes(largest_min_mem_reservation.second), PrintBytes(required_mem_limit)); : return true; : } : } factored this out http://gerrit.cloudera.org:8080/#/c/11157/6/be/src/scheduling/admission-controller.cc@1042 PS6, Line 1042: : if( > missing whitespace Done http://gerrit.cloudera.org:8080/#/c/11157/6/be/src/scheduling/admission-controller.cc@1104 PS6, Line 1104: if (!has_query_option || !pool_cfg.strict_min_max_query_mem_limit) { > Is it worth DCHECKING in this branch that min_query_mem_limit <= max_query_ Done http://gerrit.cloudera.org:8080/#/c/11157/6/be/src/scheduling/admission-controller.cc@1105 PS6, Line 1105: if (pool_cfg.min_query_mem_limit > 0) > Need braces around conditional body. Done http://gerrit.cloudera.org:8080/#/c/11157/6/be/src/scheduling/query-schedule.h File be/src/scheduling/query-schedule.h: http://gerrit.cloudera.org:8080/#/c/11157/6/be/src/scheduling/query-schedule.h@141 PS6, Line 141: /// or is computed by Scheduler and the admission controller. > Probably worth elaborating a bit - the actual schedule is computed by the S Done http://gerrit.cloudera.org:8080/#/c/11157/6/be/src/scheduling/query-schedule.h@219 PS6, Line 219: const > unnecessary const - we're returning by value Done http://gerrit.cloudera.org:8080/#/c/11157/6/be/src/scheduling/query-schedule.h@221 PS6, Line 221: const > unnecessary const - we're returning by value Done http://gerrit.cloudera.org:8080/#/c/11157/6/be/src/scheduling/query-schedule.h@223 PS6, Line 223: const > unnecessary const - we're returning by value Done http://gerrit.cloudera.org:8080/#/c/11157/6/be/src/scheduling/query-schedule.h@291 PS6, Line 291: /// Set by the admission controller on successful admission. > I'm still not sold on adding these to the schedule. I liked the the schedul Thats right, The alternatives were to pass these variables around separately or add them to some other query specific data structure like query context,etc. Neither seemed a better alternative. I also struggled with this decision too but at the end I think it made sense to put this in the schedule as in the future, if we decide to set these separately for every backend then it would make sense to add them to the per backend params which is a part of this class. http://gerrit.cloudera.org:8080/#/c/11157/6/be/src/scheduling/query-schedule.h@293 PS6, Line 293: int64_t per_backend_mem_limit_ = 0; > What's the concurrency story for these variables? It's not thread-safe but yes, this is accessed later, only by the coordinator and that too an immutable reference to it. As a part of the CRS cleanup, we can omit access to the schedule since the only time its accessed through CRS is to get the list of hosts that the query is scheduled to run on. http://gerrit.cloudera.org:8080/#/c/11157/6/fe/src/test/resources/fair-scheduler-mem-limit-test.xml File fe/src/test/resources/fair-scheduler-mem-limit-test.xml: PS6: > Maybe rename files to mem-limit-test-fair-scheduler.xml so that the related Done http://gerrit.cloudera.org:8080/#/c/11157/6/fe/src/test/resources/fair-scheduler-mem-limit-test.xml@6 PS6, Line 6: <maxResources>2500 mb, 2 vcores</maxResources> > Maybe remove the vcores since it's not needed for the test? Done http://gerrit.cloudera.org:8080/#/c/11157/6/testdata/workloads/functional-query/queries/QueryTest/admission-max-min-mem-limits.test File testdata/workloads/functional-query/queries/QueryTest/admission-max-min-mem-limits.test: PS6: > Do we have full code coverage for the branches added to admission-controlle Thanks for the tip, I'll get in touch with Joe. http://gerrit.cloudera.org:8080/#/c/11157/6/testdata/workloads/functional-query/queries/QueryTest/admission-max-min-mem-limits.test@1 PS6, Line 1: ==== > Can you add a test that runs the same query with and without num_nodes=1 an Done http://gerrit.cloudera.org:8080/#/c/11157/6/testdata/workloads/functional-query/queries/QueryTest/admission-max-min-mem-limits.test@5 PS6, Line 5: # check if mem_admitted is same as mem_estimate > Maybe worth mentioning that the query runs on a single node? Done http://gerrit.cloudera.org:8080/#/c/11157/6/testdata/workloads/functional-query/queries/QueryTest/admission-max-min-mem-limits.test@11 PS6, Line 11: row_regex: .*.* > Not sure what this is checking for? forgot to remove http://gerrit.cloudera.org:8080/#/c/11157/6/tests/custom_cluster/test_admission_controller.py File tests/custom_cluster/test_admission_controller.py: http://gerrit.cloudera.org:8080/#/c/11157/6/tests/custom_cluster/test_admission_controller.py@177 PS6, Line 177: """Creates a copy of the config files and uses those to start impala.""" > Doesn't it just return the argument list? Should the function be renamed ac merged the two http://gerrit.cloudera.org:8080/#/c/11157/6/tests/custom_cluster/test_admission_controller.py@179 PS6, Line 179: resources_dir = os.path.join(impalad_home, "fe", "src", "test", "resources") > Seems like we should factor out these common things like impalad_home and r Done http://gerrit.cloudera.org:8080/#/c/11157/6/tests/custom_cluster/test_admission_controller.py@697 PS6, Line 697: # Set num_nodes to 1 since its easier to see one-to-one mapping of per_host and > Oh i see.. this makes sense for targeted tests but we should also have some the tests with num_nodes!=1 should already have been covered under previous tests since the code to get cluster mem is orthogonal to what this test is targeting. I looked at the tests and it seems like the ones that check rejection are there in "test_memory_rejection" but the ones that check for queued reason arnt (will do that as a part of IMPALA-3600) http://gerrit.cloudera.org:8080/#/c/11157/6/tests/custom_cluster/test_admission_controller.py@720 PS6, Line 720: sleep(1) > Can we poll the query state or similar instead of sleeping? Done http://gerrit.cloudera.org:8080/#/c/11157/6/tests/custom_cluster/test_admission_controller.py@725 PS6, Line 725: # Change config to be invalid. > Maybe the config file modification should be a helper function? Actually ma Done. I also think that moving the stress test out of this file makes sense. I ll take that up as a part of IMPALA-7454 http://gerrit.cloudera.org:8080/#/c/11157/6/tests/custom_cluster/test_admission_controller.py@760 PS6, Line 760: """Helper method that constantly sends a query to the pool that will be rejected but > Maybe explicitly mention how pool_name, metric_str and target_val fit into Done http://gerrit.cloudera.org:8080/#/c/11157/6/tests/custom_cluster/test_admission_controller.py@781 PS6, Line 781: def __write_xml_to_file(self, xml_root, file_name): > Writing to the file is non-atomic, so it seems like the Impalad could see a Done -- 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: 6 Gerrit-Owner: Bikramjeet Vig <bikramjeet....@cloudera.com> Gerrit-Reviewer: Bikramjeet Vig <bikramjeet....@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Comment-Date: Fri, 17 Aug 2018 00:15:48 +0000 Gerrit-HasComments: Yes
