Tim Armstrong has posted comments on this change. Change subject: IMPALA-4833: Compute precise per-host reservation size ......................................................................
Patch Set 2: (6 comments) http://gerrit.cloudera.org:8080/#/c/7630/2/be/src/runtime/coordinator-backend-state.cc File be/src/runtime/coordinator-backend-state.cc: Line 63: //const vector<const FInstanceExecParams*>& instance_params_list, Remove? http://gerrit.cloudera.org:8080/#/c/7630/2/be/src/scheduling/query-schedule.h File be/src/scheduling/query-schedule.h: PS2, Line 55: FInstanceExecParams* Maybe make it const& since it can't be null? Line 251: /// TODO: Replace uses of unique_hosts_ with per_backend_exec_params. It looks like there are a limited number of callsites, can we just do this cleanup now? http://gerrit.cloudera.org:8080/#/c/7630/2/be/src/scheduling/scheduler.cc File be/src/scheduling/scheduler.cc: Line 688: // TODO: Move to admission control, it doesn't need to be in the Scheduler. What if admission control is disabled? I agree it doesn't seem like it fits in scheduling though. http://gerrit.cloudera.org:8080/#/c/7630/1/common/thrift/ImpalaInternalService.thrift File common/thrift/ImpalaInternalService.thrift: Line 484: // required in V1 > The default is effectively optional: Yeah this is kind of weird - I get that it makes sense if we have clients speaking different versions of the protocol but we always assume our Impala daemon versions match afaik. Oh well. http://gerrit.cloudera.org:8080/#/c/7630/2/tests/custom_cluster/test_mem_reservations.py File tests/custom_cluster/test_mem_reservations.py: Line 38: config_map = {'DEFAULT_SPILLABLE_BUFFER_SIZE': '33554432', The choice of queries, params, etc needs some explanation. -- To view, visit http://gerrit.cloudera.org:8080/7630 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Idbcd1e9b1be14edc4017b4907e83f9d56059fbac Gerrit-PatchSet: 2 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Matthew Jacobs <m...@cloudera.com> Gerrit-Reviewer: Matthew Jacobs <m...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-HasComments: Yes
