impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Matthew Jacobs (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-4833: Compute precise per-host reservation size
Date Fri, 11 Aug 2017 18:13:51 GMT
Matthew Jacobs 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?
Done


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?
Can't do it without some other trickery: https://stackoverflow.com/questions/10531010/const-references-in-stdvector-elements


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 
Done


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 
Done


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?
We don't really need the request pool for anything else anymore. It used to live here because
the Llama path used it as well, and the Llama/YARN reservation happened outside of AC. That
said, if I move this I'd try not to change the behavior, i.e. even if admission control is
disabled I'll still resolve the request pool since we do that today.


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.
Done


-- 
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 <mj@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message