You are viewing a plain text version of this content. The canonical link for it is here.

Posted to reviews@impala.apache.org by "Matthew Jacobs (Code Review)" <ge...@cloudera.org> on 2017/08/15 17:14:13 UTC

[Impala-ASF-CR] IMPALA-5644: Reject queries if min reservation is too large

Matthew Jacobs has uploaded a new change for review.

  http://gerrit.cloudera.org:8080/7678

Change subject: IMPALA-5644: Reject queries if min reservation is too large
......................................................................

IMPALA-5644: Reject queries if min reservation is too large

Rejects queries during admission control if:
* the largest (across all backends) min buffer reservation is
  greater than the query mem_limit
* the sum of the min buffer reservations across the cluster
  is larger than the pool max mem resources

There are some other interesting cases to consider later:
* every per-backend min buffer reservation is less than the
  associated backend's process mem_limit; the current
  admission control code doesn't know about other backend's
  proc mem_limits.

Some minor code refactoring in admission-controller and
query-state.

Testing:
* Added new test cases in test_admission_controller.py

Change-Id: Iabe87ce8f460356cfe4d1be4d7092c5900f9d79b
TODO: Consider more help text.
---
M be/src/runtime/bufferpool/CMakeLists.txt
A be/src/runtime/bufferpool/reservation-util.cc
A be/src/runtime/bufferpool/reservation-util.h
M be/src/runtime/query-state.cc
M be/src/scheduling/admission-controller.cc
M be/src/scheduling/admission-controller.h
M be/src/scheduling/scheduler.h
M common/thrift/generate_error_codes.py
M tests/custom_cluster/test_admission_controller.py
9 files changed, 213 insertions(+), 55 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/78/7678/1
-- 
To view, visit http://gerrit.cloudera.org:8080/7678
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: Iabe87ce8f460356cfe4d1be4d7092c5900f9d79b
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Matthew Jacobs <mj...@cloudera.com>

[Impala-ASF-CR] IMPALA-5644: Reject queries if min reservation is too large

Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.

Tim Armstrong has posted comments on this change.

Change subject: IMPALA-5644: Reject queries if min reservation is too large
......................................................................


Patch Set 4:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/7678/4/be/src/runtime/query-state.cc
File be/src/runtime/query-state.cc:

PS4, Line 153: if (query_options().__isset.buffer_pool_limit
             :       && query_options().buffer_pool_limit > 0) {
             :     max_reservation = query_options().buffer_pool_limit;
             :   }
> Question for Tim & Dan:
I tend to think of buffer_pool_limit as advanced option to directly override the default buffer pool limit, instead of being an additional constraint.


http://gerrit.cloudera.org:8080/#/c/7678/4/be/src/scheduling/admission-controller.cc
File be/src/scheduling/admission-controller.cc:

Line 125:     "more information.";
> That's right, at least not as we currently think about admission control. I
Yeah this overall seemed simplest since at this point we've already resolved the resource pool and figured out the applicable limits.

It seemed ok that it would fail later with admission control disabled, since that is an unusual suboptimal configuration.


-- 
To view, visit http://gerrit.cloudera.org:8080/7678
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Iabe87ce8f460356cfe4d1be4d7092c5900f9d79b
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5644: Reject queries if min reservation is too large

Posted by "Matthew Jacobs (Code Review)" <ge...@cloudera.org>.

Matthew Jacobs has uploaded a new patch set (#3).

Change subject: IMPALA-5644: Reject queries if min reservation is too large
......................................................................

IMPALA-5644: Reject queries if min reservation is too large

Rejects queries during admission control if:
* the largest (across all backends) min buffer reservation is
  greater than the query mem_limit or buffer_pool_limit
* the sum of the min buffer reservations across the cluster
  is larger than the pool max mem resources

There are some other interesting cases to consider later:
* every per-backend min buffer reservation is less than the
  associated backend's process mem_limit; the current
  admission control code doesn't know about other backend's
  proc mem_limits.

Some minor code refactoring in admission-controller and
query-state.

Testing:
* Added new test cases in test_admission_controller.py

Change-Id: Iabe87ce8f460356cfe4d1be4d7092c5900f9d79b
---
M be/src/runtime/bufferpool/CMakeLists.txt
M be/src/runtime/bufferpool/reservation-tracker-test.cc
A be/src/runtime/bufferpool/reservation-util.cc
A be/src/runtime/bufferpool/reservation-util.h
M be/src/runtime/query-state.cc
M be/src/scheduling/admission-controller.cc
M be/src/scheduling/admission-controller.h
M be/src/scheduling/scheduler.h
M common/thrift/generate_error_codes.py
M tests/custom_cluster/test_admission_controller.py
10 files changed, 279 insertions(+), 64 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/78/7678/3
-- 
To view, visit http://gerrit.cloudera.org:8080/7678
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iabe87ce8f460356cfe4d1be4d7092c5900f9d79b
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>

[Impala-ASF-CR] IMPALA-5644,IMPALA-5810: Min reservation improvements

Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.

Tim Armstrong has posted comments on this change.

Change subject: IMPALA-5644,IMPALA-5810: Min reservation improvements
......................................................................


Patch Set 4:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/7678/5/be/src/runtime/bufferpool/reservation-util.h
File be/src/runtime/bufferpool/reservation-util.h:

PS5, Line 32:  allocated to buffer reservations,
> I think it's the latter.
That seems clearer.


PS5, Line 36: 
            :   /// The limit on reservations is co
> I think this is providing examples of things that use the memory that is le
No there isn't a comment like that. I think this is the only place. I guess we should be a little careful with how we word it because it's subject to change.

It might be worth referencing this JIRA IMPALA-4834 "All operators should operate within a memory constraint" since that will be driving conversion of things from buffer pool to non-buffer pool memory.


http://gerrit.cloudera.org:8080/#/c/7678/4/be/src/scheduling/admission-controller.cc
File be/src/scheduling/admission-controller.cc:

PS4, Line 429: GetBufferReservationLimitFromMemLimit
> This was actually my intention originally, and that's why I asked the quest
Ah yeah, I think this should be an else-if.


-- 
To view, visit http://gerrit.cloudera.org:8080/7678
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Iabe87ce8f460356cfe4d1be4d7092c5900f9d79b
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5644,IMPALA-5810: Min reservation improvements

Posted by "Matthew Jacobs (Code Review)" <ge...@cloudera.org>.

Hello Dan Hecht, Tim Armstrong,

I'd like you to reexamine a change.  Please visit

    http://gerrit.cloudera.org:8080/7678

to look at the new patch set (#6).

Change subject: IMPALA-5644,IMPALA-5810: Min reservation improvements
......................................................................

IMPALA-5644,IMPALA-5810: Min reservation improvements

Rejects queries during admission control if:
* the largest (across all backends) min buffer reservation is
  greater than the query mem_limit or buffer_pool_limit
* the sum of the min buffer reservations across the cluster
  is larger than the pool max mem resources

There are some other interesting cases to consider later:
* every per-backend min buffer reservation is less than the
  associated backend's process mem_limit; the current
  admission control code doesn't know about other backend's
  proc mem_limits.

Also reduces minimum non-reservation memory (IMPALA-5810).
See the JIRA for experimental results that show this
slightly improves min memory requirements for small queries.
One reason to tweak this is to compensate for the fact that
BufferedBlockMgr didn't count small buffers against the
BlockMgr limit, but BufferPool counts all buffers against
it.

Testing:
* Adds new test cases in test_admission_controller.py
* Adds BE tests in reservation-tracker-test for the
  reservation-util code.

Change-Id: Iabe87ce8f460356cfe4d1be4d7092c5900f9d79b
---
M be/src/runtime/bufferpool/CMakeLists.txt
M be/src/runtime/bufferpool/reservation-tracker-test.cc
A be/src/runtime/bufferpool/reservation-util.cc
A be/src/runtime/bufferpool/reservation-util.h
M be/src/runtime/query-state.cc
M be/src/scheduling/admission-controller.cc
M be/src/scheduling/admission-controller.h
M be/src/scheduling/scheduler.h
M common/thrift/generate_error_codes.py
M tests/custom_cluster/test_admission_controller.py
10 files changed, 289 insertions(+), 64 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/78/7678/6
-- 
To view, visit http://gerrit.cloudera.org:8080/7678
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iabe87ce8f460356cfe4d1be4d7092c5900f9d79b
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>

[Impala-ASF-CR] IMPALA-5644,IMPALA-5810: Min reservation improvements

Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.

Tim Armstrong has posted comments on this change.

Change subject: IMPALA-5644,IMPALA-5810: Min reservation improvements
......................................................................


Patch Set 8: Code-Review+2

-- 
To view, visit http://gerrit.cloudera.org:8080/7678
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Iabe87ce8f460356cfe4d1be4d7092c5900f9d79b
Gerrit-PatchSet: 8
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5644,IMPALA-5810: Min reservation improvements

Posted by "Matthew Jacobs (Code Review)" <ge...@cloudera.org>.

Matthew Jacobs has posted comments on this change.

Change subject: IMPALA-5644,IMPALA-5810: Min reservation improvements
......................................................................

Patch Set 6:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/7678/5/be/src/runtime/bufferpool/reservation-util.h
File be/src/runtime/bufferpool/reservation-util.h:

Line 41: /// (i.e. not accounted by buffer reservation trackers) stays within
> This TODO is inconsistent with the others. I dont' think it really matters
Done

http://gerrit.cloudera.org:8080/#/c/7678/6/be/src/runtime/bufferpool/reservation-util.h
File be/src/runtime/bufferpool/reservation-util.h:

Line 18: // This file includes constants used for buffer management.
> Maybe remove or combine with class comment?
Done

Line 37: /// determine how much memory will get used for reserved memory vs unreserved memory.
> The last part sounds like we're partitioning between the two, but we're rea
Ok thanks. I took most of that but modified a bit.

PS6, Line 64: hueristic
> heuristic
Done

--
To view, visit http://gerrit.cloudera.org:8080/7678
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Iabe87ce8f460356cfe4d1be4d7092c5900f9d79b
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5644,IMPALA-5810: Min reservation improvements

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.

Impala Public Jenkins has posted comments on this change.

Change subject: IMPALA-5644,IMPALA-5810: Min reservation improvements
......................................................................


Patch Set 10:

Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/1114/

-- 
To view, visit http://gerrit.cloudera.org:8080/7678
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Iabe87ce8f460356cfe4d1be4d7092c5900f9d79b
Gerrit-PatchSet: 10
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5644: Reject queries if min reservation is too large

Posted by "Matthew Jacobs (Code Review)" <ge...@cloudera.org>.

Matthew Jacobs has posted comments on this change.

Change subject: IMPALA-5644: Reject queries if min reservation is too large
......................................................................


Patch Set 4:

I filed IMPALA-5806

-- 
To view, visit http://gerrit.cloudera.org:8080/7678
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Iabe87ce8f460356cfe4d1be4d7092c5900f9d79b
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5644,IMPALA-5810: Min reservation improvements

Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.

Tim Armstrong has posted comments on this change.

Change subject: IMPALA-5644,IMPALA-5810: Min reservation improvements
......................................................................

Patch Set 6: Code-Review+2

(5 comments)

http://gerrit.cloudera.org:8080/#/c/7678/5/be/src/runtime/bufferpool/reservation-util.h
File be/src/runtime/bufferpool/reservation-util.h:

Line 41: /// (i.e. not accounted by buffer reservation trackers) stays within
This TODO is inconsistent with the others. I dont' think it really matters though.

http://gerrit.cloudera.org:8080/#/c/7678/6/be/src/runtime/bufferpool/reservation-util.h
File be/src/runtime/bufferpool/reservation-util.h:

Line 18: // This file includes constants used for buffer management.
Maybe remove or combine with class comment?

Line 30: /// There are currently two classes of memory: reserved memory (i.e. memory that is
This is a big improvement.

Line 37: /// determine how much memory will get used for reserved memory vs unreserved memory.
The last part sounds like we're partitioning between the two, but we're really just bounding reserved memory.

Maybe instead end with "...used to determine an upper bound on reserved memory. Operators are designed to operate reliably when they are up against a hard constraint on reserved memory (e.g. staying under a limit by spilling), but will generally fail if they hit a limit when trying to allocate unreserved memory. Thus we need to ensure there is always space left in the query memory limit for unreserved memory."

PS6, Line 64: hueristic
heuristic

--
To view, visit http://gerrit.cloudera.org:8080/7678
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

[Impala-ASF-CR] IMPALA-5644: Reject queries if min reservation is too large

Posted by "Matthew Jacobs (Code Review)" <ge...@cloudera.org>.

Matthew Jacobs has posted comments on this change.

Change subject: IMPALA-5644: Reject queries if min reservation is too large
......................................................................


Patch Set 4:

(8 comments)

http://gerrit.cloudera.org:8080/#/c/7678/4/be/src/runtime/bufferpool/reservation-util.h
File be/src/runtime/bufferpool/reservation-util.h:

PS4, Line 44: buffer reservation limi
> is it clear that this is talking about the query limit as opposed to the bu
Done


PS4, Line 53: minimum
> i think that should be deleted, i.e. you might want to ask this for a poten
Done


PS4, Line 57: GetMemLimitFromBufferReservation
> the name makes it sound like an inverse of GetBufferReservationLimitFromMem
Done


http://gerrit.cloudera.org:8080/#/c/7678/4/be/src/runtime/query-state.cc
File be/src/runtime/query-state.cc:

PS4, Line 153: if (query_options().__isset.buffer_pool_limit
             :       && query_options().buffer_pool_limit > 0) {
             :     max_reservation = query_options().buffer_pool_limit;
             :   }
Question for Tim & Dan:

Should this really be exclusive w/ checking the mem limit? Don't we want:


max_reservation = mem_limit == -1 ? max_int64 : ResUtil::GetBufferReservationLimitFromMemLimit(mem_limit)
if (buffer_pool_limit is set):
  max_reservation = min(query_options.buffer_pool_limit,
     max_reservation)


http://gerrit.cloudera.org:8080/#/c/7678/4/be/src/scheduling/admission-controller.cc
File be/src/scheduling/admission-controller.cc:

PS4, Line 119: greater than
> at least? (off by one?) 
Done


Line 125:     "more information.";
> both of these two reasons aren't really admission control specific, right? 
That's right, at least not as we currently think about admission control. It could go somewhere else but I thought it made sense to do all of our resource-related query 'rejecting' in one place, and I think it makes sense for admission control to be that place.

I also don't think people should be running with AC disabled anymore. It's better to leave AC enabled but w/ unlimited limits (which is the default behavior). I think there shouldn't be many users with AC explicitly disabled anymore. Maybe I'll mark those flags as deprecated and remove in Impala 3.0?


PS4, Line 428: buffer_reservation_limit
> might be nice to name this consistently with query-state.cc naming, to make
Done


PS4, Line 429: GetBufferReservationLimitFromMemLimit
> what about the case of buffer_pool_limit set explicitly?
Do you mean something besides what gets handled on l416-l424?


-- 
To view, visit http://gerrit.cloudera.org:8080/7678
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Iabe87ce8f460356cfe4d1be4d7092c5900f9d79b
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5644,IMPALA-5810: Min reservation improvements

Posted by "Matthew Jacobs (Code Review)" <ge...@cloudera.org>.

Matthew Jacobs has posted comments on this change.

Change subject: IMPALA-5644,IMPALA-5810: Min reservation improvements
......................................................................


Patch Set 10: Code-Review+2

String msg changed and an existing test needed to be updated

-- 
To view, visit http://gerrit.cloudera.org:8080/7678
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Iabe87ce8f460356cfe4d1be4d7092c5900f9d79b
Gerrit-PatchSet: 10
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5644,IMPALA-5810: Min reservation improvements

Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.

Dan Hecht has posted comments on this change.

Change subject: IMPALA-5644,IMPALA-5810: Min reservation improvements
......................................................................

Patch Set 5: Code-Review+2

(4 comments)

Code looks fine but I think those (pre-existing) comments could use some fixing/clarifying. I'll be on PTO tomorrow, so feel free to work with Tim to get that finished up (or I can look on Monday).

http://gerrit.cloudera.org:8080/#/c/7678/5/be/src/runtime/bufferpool/reservation-util.h
File be/src/runtime/bufferpool/reservation-util.h:

PS5, Line 32: allocated to buffer reservations,
After thinking about these variables more, this paragraph doesn't seem right. does this amount actually get set aside to be used only be the buffer pool (i.e. allocated to buffer reservations), or does it just set the upper bound on the amount of buffer pool memory that the query can use?

PS5, Line 36: or
: /// scans, exprs, row batches, etc.
I don't follow that. we don't use buffer reservations for those yet.

Oh, do you mean the amount of memory (for scans, exprs, row batches, etc) that should not be usable by the buffer pool?

This comment applies to both of these variables though, right? So maybe we should have a quick high level comment explaining that memory is divided into two classes and what they are and examples of how they're used, and then the variable specific comments can just explain how they put guard rails on each so neither is starved.

http://gerrit.cloudera.org:8080/#/c/7678/4/be/src/scheduling/admission-controller.cc
File be/src/scheduling/admission-controller.cc:

Line 125: "more information.";
> Yeah this overall seemed simplest since at this point we've already resolve
I think we should soon make it so you can't disable admission control. Not sure that even has to wait for 3.0 if the default configuration is logically equivalent (other then fail fast vs fail slow).

PS4, Line 429: GetReservationLimitFromMemLimit(mem_l
> Do you mean something besides what gets handled on l416-l424?
I missed that. But should line 425 be an else-if then?

--
To view, visit http://gerrit.cloudera.org:8080/7678
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Iabe87ce8f460356cfe4d1be4d7092c5900f9d79b
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5644: Reject queries if min reservation is too large

Posted by "Matthew Jacobs (Code Review)" <ge...@cloudera.org>.

Matthew Jacobs has posted comments on this change.

Change subject: IMPALA-5644: Reject queries if min reservation is too large
......................................................................


Patch Set 3:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/7678/2/be/src/scheduling/admission-controller.cc
File be/src/scheduling/admission-controller.cc:

Line 401:   // Compute the max (over all backends) min_reservation_bytes and the cluster total
> Yeah that's true, we do have that in the profile.
Yeah maybe this should mention the profile. Perhaps I won't point to a particular field, but something more general like appending: "See the query profile for more information."

What do you think of that at the end of the 3 new error strings?

I was going to file a JIRA to track making the schedule more observable, but JIRA isn't responding. Will do that later.


http://gerrit.cloudera.org:8080/#/c/7678/3/common/thrift/generate_error_codes.py
File common/thrift/generate_error_codes.py:

PS3, Line 332:  
> Nit: don't need space before colon
Done


-- 
To view, visit http://gerrit.cloudera.org:8080/7678
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Iabe87ce8f460356cfe4d1be4d7092c5900f9d79b
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5644,IMPALA-5810: Min reservation improvements

Posted by "Matthew Jacobs (Code Review)" <ge...@cloudera.org>.

Hello Impala Public Jenkins, Dan Hecht, Tim Armstrong,

I'd like you to reexamine a change.  Please visit

    http://gerrit.cloudera.org:8080/7678

to look at the new patch set (#10).

Change subject: IMPALA-5644,IMPALA-5810: Min reservation improvements
......................................................................

IMPALA-5644,IMPALA-5810: Min reservation improvements

Rejects queries during admission control if:
* the largest (across all backends) min buffer reservation is
  greater than the query mem_limit or buffer_pool_limit
* the sum of the min buffer reservations across the cluster
  is larger than the pool max mem resources

There are some other interesting cases to consider later:
* every per-backend min buffer reservation is less than the
  associated backend's process mem_limit; the current
  admission control code doesn't know about other backend's
  proc mem_limits.

Also reduces minimum non-reservation memory (IMPALA-5810).
See the JIRA for experimental results that show this
slightly improves min memory requirements for small queries.
One reason to tweak this is to compensate for the fact that
BufferedBlockMgr didn't count small buffers against the
BlockMgr limit, but BufferPool counts all buffers against
it.

Testing:
* Adds new test cases in test_admission_controller.py
* Adds BE tests in reservation-tracker-test for the
  reservation-util code.

Change-Id: Iabe87ce8f460356cfe4d1be4d7092c5900f9d79b
---
M be/src/runtime/bufferpool/CMakeLists.txt
M be/src/runtime/bufferpool/reservation-tracker-test.cc
A be/src/runtime/bufferpool/reservation-util.cc
A be/src/runtime/bufferpool/reservation-util.h
M be/src/runtime/query-state.cc
M be/src/scheduling/admission-controller.cc
M be/src/scheduling/admission-controller.h
M be/src/scheduling/scheduler.h
M common/thrift/generate_error_codes.py
M testdata/workloads/functional-query/queries/QueryTest/analytic-fns.test
M testdata/workloads/functional-query/queries/QueryTest/spilling-aggs.test
M tests/custom_cluster/test_admission_controller.py
M tests/query_test/test_mem_usage_scaling.py
13 files changed, 298 insertions(+), 104 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/78/7678/10
-- 
To view, visit http://gerrit.cloudera.org:8080/7678
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iabe87ce8f460356cfe4d1be4d7092c5900f9d79b
Gerrit-PatchSet: 10
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>

[Impala-ASF-CR] IMPALA-5644,IMPALA-5810: Min reservation improvements

Posted by "Matthew Jacobs (Code Review)" <ge...@cloudera.org>.

Matthew Jacobs has posted comments on this change.

Change subject: IMPALA-5644,IMPALA-5810: Min reservation improvements
......................................................................


Patch Set 8:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/7678/8/be/src/runtime/bufferpool/reservation-util.h
File be/src/runtime/bufferpool/reservation-util.h:

PS8, Line 35: .
> That's much clearer, but I think it would help to clearly say these limits 
Done


-- 
To view, visit http://gerrit.cloudera.org:8080/7678
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Iabe87ce8f460356cfe4d1be4d7092c5900f9d79b
Gerrit-PatchSet: 8
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5644: Reject queries if min reservation is too large

Posted by "Matthew Jacobs (Code Review)" <ge...@cloudera.org>.

Matthew Jacobs has uploaded a new patch set (#2).

Change subject: IMPALA-5644: Reject queries if min reservation is too large
......................................................................

IMPALA-5644: Reject queries if min reservation is too large

Rejects queries during admission control if:
* the largest (across all backends) min buffer reservation is
  greater than the query mem_limit or buffer_pool_limit
* the sum of the min buffer reservations across the cluster
  is larger than the pool max mem resources

There are some other interesting cases to consider later:
* every per-backend min buffer reservation is less than the
  associated backend's process mem_limit; the current
  admission control code doesn't know about other backend's
  proc mem_limits.

Some minor code refactoring in admission-controller and
query-state.

Testing:
* Added new test cases in test_admission_controller.py

Change-Id: Iabe87ce8f460356cfe4d1be4d7092c5900f9d79b
---
M be/src/runtime/bufferpool/CMakeLists.txt
A be/src/runtime/bufferpool/reservation-util.cc
A be/src/runtime/bufferpool/reservation-util.h
M be/src/runtime/query-state.cc
M be/src/scheduling/admission-controller.cc
M be/src/scheduling/admission-controller.h
M be/src/scheduling/scheduler.h
M common/thrift/generate_error_codes.py
M tests/custom_cluster/test_admission_controller.py
9 files changed, 242 insertions(+), 64 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/78/7678/2
-- 
To view, visit http://gerrit.cloudera.org:8080/7678
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iabe87ce8f460356cfe4d1be4d7092c5900f9d79b
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>

[Impala-ASF-CR] IMPALA-5644,IMPALA-5810: Min reservation improvements

Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.

Dan Hecht has posted comments on this change.

Change subject: IMPALA-5644,IMPALA-5810: Min reservation improvements
......................................................................


Patch Set 8: Code-Review+2

(1 comment)

http://gerrit.cloudera.org:8080/#/c/7678/8/be/src/runtime/bufferpool/reservation-util.h
File be/src/runtime/bufferpool/reservation-util.h:

PS8, Line 35: .
That's much clearer, but I think it would help to clearly say these limits apply per query (as opposed to an upper bound for the process). It's implied later, but clearer to say it explicitly up front.


-- 
To view, visit http://gerrit.cloudera.org:8080/7678
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Iabe87ce8f460356cfe4d1be4d7092c5900f9d79b
Gerrit-PatchSet: 8
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5644,IMPALA-5810: Min reservation improvements

Posted by "Matthew Jacobs (Code Review)" <ge...@cloudera.org>.

Hello Dan Hecht, Tim Armstrong,

I'd like you to reexamine a change.  Please visit

    http://gerrit.cloudera.org:8080/7678

to look at the new patch set (#7).

Change subject: IMPALA-5644,IMPALA-5810: Min reservation improvements
......................................................................

IMPALA-5644,IMPALA-5810: Min reservation improvements

Rejects queries during admission control if:
* the largest (across all backends) min buffer reservation is
  greater than the query mem_limit or buffer_pool_limit
* the sum of the min buffer reservations across the cluster
  is larger than the pool max mem resources

There are some other interesting cases to consider later:
* every per-backend min buffer reservation is less than the
  associated backend's process mem_limit; the current
  admission control code doesn't know about other backend's
  proc mem_limits.

Also reduces minimum non-reservation memory (IMPALA-5810).
See the JIRA for experimental results that show this
slightly improves min memory requirements for small queries.
One reason to tweak this is to compensate for the fact that
BufferedBlockMgr didn't count small buffers against the
BlockMgr limit, but BufferPool counts all buffers against
it.

Testing:
* Adds new test cases in test_admission_controller.py
* Adds BE tests in reservation-tracker-test for the
  reservation-util code.

Change-Id: Iabe87ce8f460356cfe4d1be4d7092c5900f9d79b
---
M be/src/runtime/bufferpool/CMakeLists.txt
M be/src/runtime/bufferpool/reservation-tracker-test.cc
A be/src/runtime/bufferpool/reservation-util.cc
A be/src/runtime/bufferpool/reservation-util.h
M be/src/runtime/query-state.cc
M be/src/scheduling/admission-controller.cc
M be/src/scheduling/admission-controller.h
M be/src/scheduling/scheduler.h
M common/thrift/generate_error_codes.py
M tests/custom_cluster/test_admission_controller.py
10 files changed, 291 insertions(+), 64 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/78/7678/7
-- 
To view, visit http://gerrit.cloudera.org:8080/7678
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iabe87ce8f460356cfe4d1be4d7092c5900f9d79b
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>

[Impala-ASF-CR] IMPALA-5644: Reject queries if min reservation is too large

Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.

Dan Hecht has posted comments on this change.

Change subject: IMPALA-5644: Reject queries if min reservation is too large
......................................................................

Patch Set 4:

(7 comments)

http://gerrit.cloudera.org:8080/#/c/7678/4/be/src/runtime/bufferpool/reservation-util.h
File be/src/runtime/bufferpool/reservation-util.h:

PS4, Line 44: buffer reservation limi
is it clear that this is talking about the query limit as opposed to the buffer-pool wide limit? I guess maybe, since we refer to the latter as the "buffer pool limit", but maybe worth clarifying.

PS4, Line 53: minimum
i think that should be deleted, i.e. you might want to ask this for a potential initial reservation that's larger than the minimum.

PS4, Line 57: GetMemLimitFromBufferReservation
the name makes it sound like an inverse of GetBufferReservationLimitFromMemLimit(), but it's a little different conceptually. How about GetMinMemLimitFromBufferReservation or GetRequiredMemLimitFromBufferReservation...

also, if you want to shorten, I think removing the word "buffer" from the name would be okay.

http://gerrit.cloudera.org:8080/#/c/7678/4/be/src/scheduling/admission-controller.cc
File be/src/scheduling/admission-controller.cc:

PS4, Line 119: greater than
at least? (off by one?)
and are we suggesting that mem_limit be set to this value? If so, maybe say that more explicitly: Set mem_limit to at least $2?

Line 125: "more information.";
both of these two reasons aren't really admission control specific, right? i'm okay with having them here though, but what happens when admission control is disabled? it fails later to get the initial reservation, I guess?

PS4, Line 428: buffer_reservation_limit
might be nice to name this consistently with query-state.cc naming, to make it clear we're talking about the same thing, which I think calls it max_reservation (or rename that one).

PS4, Line 429: GetBufferReservationLimitFromMemLimit
what about the case of buffer_pool_limit set explicitly?

--
To view, visit http://gerrit.cloudera.org:8080/7678
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Iabe87ce8f460356cfe4d1be4d7092c5900f9d79b
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5644: Reject queries if min reservation is too large

Posted by "Matthew Jacobs (Code Review)" <ge...@cloudera.org>.

Matthew Jacobs has posted comments on this change.

Change subject: IMPALA-5644: Reject queries if min reservation is too large
......................................................................


Patch Set 4: Code-Review+1

Carrying Tim's +1

-- 
To view, visit http://gerrit.cloudera.org:8080/7678
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Iabe87ce8f460356cfe4d1be4d7092c5900f9d79b
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5644,IMPALA-5810: Min reservation improvements

Posted by "Matthew Jacobs (Code Review)" <ge...@cloudera.org>.

Matthew Jacobs has posted comments on this change.

Change subject: IMPALA-5644,IMPALA-5810: Min reservation improvements
......................................................................


Patch Set 7: Code-Review+2

carrying Tim's +2

-- 
To view, visit http://gerrit.cloudera.org:8080/7678
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Iabe87ce8f460356cfe4d1be4d7092c5900f9d79b
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5644,IMPALA-5810: Min reservation improvements

Posted by "Matthew Jacobs (Code Review)" <ge...@cloudera.org>.

Hello Dan Hecht, Tim Armstrong,

I'd like you to reexamine a change.  Please visit

    http://gerrit.cloudera.org:8080/7678

to look at the new patch set (#9).

Change subject: IMPALA-5644,IMPALA-5810: Min reservation improvements
......................................................................

IMPALA-5644,IMPALA-5810: Min reservation improvements

Rejects queries during admission control if:
* the largest (across all backends) min buffer reservation is
  greater than the query mem_limit or buffer_pool_limit
* the sum of the min buffer reservations across the cluster
  is larger than the pool max mem resources

There are some other interesting cases to consider later:
* every per-backend min buffer reservation is less than the
  associated backend's process mem_limit; the current
  admission control code doesn't know about other backend's
  proc mem_limits.

Also reduces minimum non-reservation memory (IMPALA-5810).
See the JIRA for experimental results that show this
slightly improves min memory requirements for small queries.
One reason to tweak this is to compensate for the fact that
BufferedBlockMgr didn't count small buffers against the
BlockMgr limit, but BufferPool counts all buffers against
it.

Testing:
* Adds new test cases in test_admission_controller.py
* Adds BE tests in reservation-tracker-test for the
  reservation-util code.

Change-Id: Iabe87ce8f460356cfe4d1be4d7092c5900f9d79b
---
M be/src/runtime/bufferpool/CMakeLists.txt
M be/src/runtime/bufferpool/reservation-tracker-test.cc
A be/src/runtime/bufferpool/reservation-util.cc
A be/src/runtime/bufferpool/reservation-util.h
M be/src/runtime/query-state.cc
M be/src/scheduling/admission-controller.cc
M be/src/scheduling/admission-controller.h
M be/src/scheduling/scheduler.h
M common/thrift/generate_error_codes.py
M testdata/workloads/functional-query/queries/QueryTest/analytic-fns.test
M testdata/workloads/functional-query/queries/QueryTest/spilling-aggs.test
M tests/custom_cluster/test_admission_controller.py
M tests/query_test/test_mem_usage_scaling.py
13 files changed, 297 insertions(+), 103 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/78/7678/9
-- 
To view, visit http://gerrit.cloudera.org:8080/7678
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iabe87ce8f460356cfe4d1be4d7092c5900f9d79b
Gerrit-PatchSet: 9
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>

[Impala-ASF-CR] IMPALA-5644: Reject queries if min reservation is too large

Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.

Tim Armstrong has posted comments on this change.

Change subject: IMPALA-5644: Reject queries if min reservation is too large
......................................................................


Patch Set 1:

(5 comments)

http://gerrit.cloudera.org:8080/#/c/7678/1/be/src/scheduling/admission-controller.cc
File be/src/scheduling/admission-controller.cc:

PS1, Line 445: GetProcMemLimit
> I thought about it, though I think we should hold off for now. This check i
Ah ok, makes sense. Yeah I think there's a lot of opportunity for improvement.


http://gerrit.cloudera.org:8080/#/c/7678/2/be/src/scheduling/admission-controller.cc
File be/src/scheduling/admission-controller.cc:

Line 401:     }
I was wondering about whether it makes sense to report which backend had the max reservation.

My feeling is that the additional info isn't actionable so isn't worth including.


Line 430:     *rejection_reason = REASON_DISABLED_MAX_MEM_RESOURCES;
It seems like this would fit better in ReservationUtil.


Line 432:   } else if (pool_cfg.max_mem_resources > 0 &&
I think the test is only exercising the second MEM_MIN_REMAINING calculation. Might be good to have a simple unit test (e.g. in reservation-tracker-test) to check a couple more values of the calculation.


http://gerrit.cloudera.org:8080/#/c/7678/2/be/src/scheduling/admission-controller.h
File be/src/scheduling/admission-controller.h:

PS2, Line 465: set
maybe "set to a explanation of why the request was rejected".


-- 
To view, visit http://gerrit.cloudera.org:8080/7678
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Iabe87ce8f460356cfe4d1be4d7092c5900f9d79b
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5644,IMPALA-5810: Min reservation improvements

Posted by "Matthew Jacobs (Code Review)" <ge...@cloudera.org>.

Hello Dan Hecht, Tim Armstrong,

I'd like you to reexamine a change.  Please visit

    http://gerrit.cloudera.org:8080/7678

to look at the new patch set (#8).

Change subject: IMPALA-5644,IMPALA-5810: Min reservation improvements
......................................................................

IMPALA-5644,IMPALA-5810: Min reservation improvements

Rejects queries during admission control if:
* the largest (across all backends) min buffer reservation is
  greater than the query mem_limit or buffer_pool_limit
* the sum of the min buffer reservations across the cluster
  is larger than the pool max mem resources

There are some other interesting cases to consider later:
* every per-backend min buffer reservation is less than the
  associated backend's process mem_limit; the current
  admission control code doesn't know about other backend's
  proc mem_limits.

Also reduces minimum non-reservation memory (IMPALA-5810).
See the JIRA for experimental results that show this
slightly improves min memory requirements for small queries.
One reason to tweak this is to compensate for the fact that
BufferedBlockMgr didn't count small buffers against the
BlockMgr limit, but BufferPool counts all buffers against
it.

Testing:
* Adds new test cases in test_admission_controller.py
* Adds BE tests in reservation-tracker-test for the
  reservation-util code.

Change-Id: Iabe87ce8f460356cfe4d1be4d7092c5900f9d79b
---
M be/src/runtime/bufferpool/CMakeLists.txt
M be/src/runtime/bufferpool/reservation-tracker-test.cc
A be/src/runtime/bufferpool/reservation-util.cc
A be/src/runtime/bufferpool/reservation-util.h
M be/src/runtime/query-state.cc
M be/src/scheduling/admission-controller.cc
M be/src/scheduling/admission-controller.h
M be/src/scheduling/scheduler.h
M common/thrift/generate_error_codes.py
M testdata/workloads/functional-query/queries/QueryTest/analytic-fns.test
M tests/custom_cluster/test_admission_controller.py
M tests/query_test/test_mem_usage_scaling.py
12 files changed, 296 insertions(+), 102 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/78/7678/8
-- 
To view, visit http://gerrit.cloudera.org:8080/7678
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iabe87ce8f460356cfe4d1be4d7092c5900f9d79b
Gerrit-PatchSet: 8
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>

[Impala-ASF-CR] IMPALA-5644: Reject queries if min reservation is too large

Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.

Tim Armstrong has posted comments on this change.

Change subject: IMPALA-5644: Reject queries if min reservation is too large
......................................................................


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/7678/2/be/src/scheduling/admission-controller.cc
File be/src/scheduling/admission-controller.cc:

Line 401:   // Compute the max (over all backends) min_reservation_bytes and the cluster total
> Yeah maybe this should mention the profile. Perhaps I won't point to a part
WFM. It seems helpful since it's not obvious otherwise that the profile would have relevant info.


-- 
To view, visit http://gerrit.cloudera.org:8080/7678
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Iabe87ce8f460356cfe4d1be4d7092c5900f9d79b
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5644,IMPALA-5810: Min reservation improvements

Posted by "Matthew Jacobs (Code Review)" <ge...@cloudera.org>.

Matthew Jacobs has posted comments on this change.

Change subject: IMPALA-5644,IMPALA-5810: Min reservation improvements
......................................................................


Patch Set 8:

looks like test_spilling.py will still require a lot of changes

-- 
To view, visit http://gerrit.cloudera.org:8080/7678
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Iabe87ce8f460356cfe4d1be4d7092c5900f9d79b
Gerrit-PatchSet: 8
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5644: Reject queries if min reservation is too large

Posted by "Matthew Jacobs (Code Review)" <ge...@cloudera.org>.

Matthew Jacobs has posted comments on this change.

Change subject: IMPALA-5644: Reject queries if min reservation is too large
......................................................................

Patch Set 1:

(5 comments)

http://gerrit.cloudera.org:8080/#/c/7678/1/be/src/runtime/bufferpool/reservation-util.cc
File be/src/runtime/bufferpool/reservation-util.cc:

Line 23: const double ReservationUtil::RESERVATION_MEM_FRACTION = 0.8;
> We don't normally indent inside namespaces.
Done

http://gerrit.cloudera.org:8080/#/c/7678/1/be/src/runtime/query-state.cc
File be/src/runtime/query-state.cc:

Line 153: if (query_options().__isset.buffer_pool_limit
> The logic in admission control should also take into account buffer_pool_li
Good point. I'll update AC and remove the broken test case. I'll be sure to wait & rebase my change on top of the change w/ your new test.

PS1, Line 155: max_reservation = query_options().buffer_pool_limit;
shouldn't max_reservation be the the minimum of query_options().buffer_pool_limit (if set) and mem_limit (if set)?

This code implies that you can set a buffer_pool_limit > mem_limit and bypass the mem_limit here. I'd assume the query would fail later.

http://gerrit.cloudera.org:8080/#/c/7678/1/be/src/scheduling/admission-controller.cc
File be/src/scheduling/admission-controller.cc:

Line 428: // Checks related to pool max_mem_resources:
> Is there a reason behind the order of the checks? Might be helpful to docum
Not particularly, though I tried to think about what checks you'd want to be reported as failing first since the user will only get an error with the first check to fail. I'll add a comment at the top of this function.

PS1, Line 445: GetProcMemLimit
> Should we also be comparing the reservation against the process memory limi
I thought about it, though I think we should hold off for now. This check isn't great because it's wrong if there are different process mem_limits across the cluster. Lets start planning the next steps for admission control and see how we can fit it into the roadmap.

--
To view, visit http://gerrit.cloudera.org:8080/7678
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Iabe87ce8f460356cfe4d1be4d7092c5900f9d79b
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5644: Reject queries if min reservation is too large

Posted by "Matthew Jacobs (Code Review)" <ge...@cloudera.org>.

Matthew Jacobs has posted comments on this change.

Change subject: IMPALA-5644: Reject queries if min reservation is too large
......................................................................


Patch Set 2:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/7678/2/be/src/scheduling/admission-controller.cc
File be/src/scheduling/admission-controller.cc:

Line 401:   // Compute the max (over all backends) min_reservation_bytes and the cluster total
> I was wondering about whether it makes sense to report which backend had th
Yeah it's a good idea, I started playing around with doing that earlier but also realized there wasn't much to do with it. I think what the user really needs is a way to get a good dump of all the info affecting these decisions as part of the profile. The profile of these rejected queries will have the "Per Host Min Reservation: " line in the profile, but unfortunately it may still be hard to figure out what to do with that. We may need a way to see something like the plan annotated with the post-scheduling information.


Line 430:       int64_t recommended_mem_limit = std::max<int64_t>(
> It seems like this would fit better in ReservationUtil.
Done


Line 432:           max_min_reservation_bytes + ReservationUtil::RESERVATION_MEM_MIN_REMAINING);
> I think the test is only exercising the second MEM_MIN_REMAINING calculatio
Done


http://gerrit.cloudera.org:8080/#/c/7678/2/be/src/scheduling/admission-controller.h
File be/src/scheduling/admission-controller.h:

PS2, Line 465: set
> maybe "set to a explanation of why the request was rejected".
Done


-- 
To view, visit http://gerrit.cloudera.org:8080/7678
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Iabe87ce8f460356cfe4d1be4d7092c5900f9d79b
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5644: Reject queries if min reservation is too large

Posted by "Matthew Jacobs (Code Review)" <ge...@cloudera.org>.

Matthew Jacobs has posted comments on this change.

Change subject: IMPALA-5644: Reject queries if min reservation is too large
......................................................................


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/7678/2/be/src/scheduling/admission-controller.cc
File be/src/scheduling/admission-controller.cc:

Line 401:   // Compute the max (over all backends) min_reservation_bytes and the cluster total
> WFM. It seems helpful since it's not obvious otherwise that the profile wou
Done


-- 
To view, visit http://gerrit.cloudera.org:8080/7678
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Iabe87ce8f460356cfe4d1be4d7092c5900f9d79b
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5644,IMPALA-5810: Min reservation improvements

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.

Impala Public Jenkins has posted comments on this change.

Change subject: IMPALA-5644,IMPALA-5810: Min reservation improvements
......................................................................


Patch Set 9:

Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/1109/

-- 
To view, visit http://gerrit.cloudera.org:8080/7678
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Iabe87ce8f460356cfe4d1be4d7092c5900f9d79b
Gerrit-PatchSet: 9
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5644,IMPALA-5810: Min reservation improvements

Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.

Tim Armstrong has posted comments on this change.

Change subject: IMPALA-5644,IMPALA-5810: Min reservation improvements
......................................................................


Patch Set 5: Code-Review+1

-- 
To view, visit http://gerrit.cloudera.org:8080/7678
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Iabe87ce8f460356cfe4d1be4d7092c5900f9d79b
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5644,IMPALA-5810: Min reservation improvements

Posted by "Matthew Jacobs (Code Review)" <ge...@cloudera.org>.

Matthew Jacobs has posted comments on this change.

Change subject: IMPALA-5644,IMPALA-5810: Min reservation improvements
......................................................................


Patch Set 9: Code-Review+2

-- 
To view, visit http://gerrit.cloudera.org:8080/7678
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Iabe87ce8f460356cfe4d1be4d7092c5900f9d79b
Gerrit-PatchSet: 9
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5644,IMPALA-5810: Min reservation improvements

Posted by "Matthew Jacobs (Code Review)" <ge...@cloudera.org>.

Hello Tim Armstrong,

I'd like you to reexamine a change.  Please visit

    http://gerrit.cloudera.org:8080/7678

to look at the new patch set (#5).

Change subject: IMPALA-5644,IMPALA-5810: Min reservation improvements
......................................................................

IMPALA-5644,IMPALA-5810: Min reservation improvements

Rejects queries during admission control if:
* the largest (across all backends) min buffer reservation is
  greater than the query mem_limit or buffer_pool_limit
* the sum of the min buffer reservations across the cluster
  is larger than the pool max mem resources

There are some other interesting cases to consider later:
* every per-backend min buffer reservation is less than the
  associated backend's process mem_limit; the current
  admission control code doesn't know about other backend's
  proc mem_limits.

Also reduces minimum non-reservation memory (IMPALA-5810).
See the JIRA for experimental results that show this
slightly improves min memory requirements for small queries.
One reason to tweak this is to compensate for the fact that
BufferedBlockMgr didn't count small buffers against the
BlockMgr limit, but BufferPool counts all buffers against
it.

Testing:
* Adds new test cases in test_admission_controller.py
* Adds BE tests in reservation-tracker-test for the
  reservation-util code.

Change-Id: Iabe87ce8f460356cfe4d1be4d7092c5900f9d79b
---
M be/src/runtime/bufferpool/CMakeLists.txt
M be/src/runtime/bufferpool/reservation-tracker-test.cc
A be/src/runtime/bufferpool/reservation-util.cc
A be/src/runtime/bufferpool/reservation-util.h
M be/src/runtime/query-state.cc
M be/src/scheduling/admission-controller.cc
M be/src/scheduling/admission-controller.h
M be/src/scheduling/scheduler.h
M common/thrift/generate_error_codes.py
M tests/custom_cluster/test_admission_controller.py
10 files changed, 279 insertions(+), 64 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/78/7678/5
-- 
To view, visit http://gerrit.cloudera.org:8080/7678
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iabe87ce8f460356cfe4d1be4d7092c5900f9d79b
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>

[Impala-ASF-CR] IMPALA-5644,IMPALA-5810: Min reservation improvements

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.

Impala Public Jenkins has posted comments on this change.

Change subject: IMPALA-5644,IMPALA-5810: Min reservation improvements
......................................................................


Patch Set 10: Verified+1

-- 
To view, visit http://gerrit.cloudera.org:8080/7678
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Iabe87ce8f460356cfe4d1be4d7092c5900f9d79b
Gerrit-PatchSet: 10
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5644,IMPALA-5810: Min reservation improvements

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.

Impala Public Jenkins has posted comments on this change.

Change subject: IMPALA-5644,IMPALA-5810: Min reservation improvements
......................................................................


Patch Set 9: Verified-1

Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/1109/

-- 
To view, visit http://gerrit.cloudera.org:8080/7678
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Iabe87ce8f460356cfe4d1be4d7092c5900f9d79b
Gerrit-PatchSet: 9
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5644: Reject queries if min reservation is too large

Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.

Tim Armstrong has posted comments on this change.

Change subject: IMPALA-5644: Reject queries if min reservation is too large
......................................................................


Patch Set 3: Code-Review+1

(2 comments)

http://gerrit.cloudera.org:8080/#/c/7678/2/be/src/scheduling/admission-controller.cc
File be/src/scheduling/admission-controller.cc:

Line 401:   // Compute the max (over all backends) min_reservation_bytes and the cluster total
> Yeah it's a good idea, I started playing around with doing that earlier but
Yeah that's true, we do have that in the profile.

Would it make sense to refer to that profile line in the error message?

Printing the schedule in a human-readable format as part of the explain plan isn't such a crazy idea :).

I guess it's not as simple if we ever change the scheduling logic so that it's not a discrete step before admission control.


http://gerrit.cloudera.org:8080/#/c/7678/3/common/thrift/generate_error_codes.py
File common/thrift/generate_error_codes.py:

PS3, Line 332:  
Nit: don't need space before colon


-- 
To view, visit http://gerrit.cloudera.org:8080/7678
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Iabe87ce8f460356cfe4d1be4d7092c5900f9d79b
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5644,IMPALA-5810: Min reservation improvements

Posted by "Matthew Jacobs (Code Review)" <ge...@cloudera.org>.

Matthew Jacobs has posted comments on this change.

Change subject: IMPALA-5644,IMPALA-5810: Min reservation improvements
......................................................................


Patch Set 5:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/7678/5/be/src/runtime/bufferpool/reservation-util.h
File be/src/runtime/bufferpool/reservation-util.h:

PS5, Line 36: or
            :   /// scans, exprs, row batches, etc.
> No there isn't a comment like that. I think this is the only place. I guess
Ok, I made some changes, hopefully that helps.


-- 
To view, visit http://gerrit.cloudera.org:8080/7678
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Iabe87ce8f460356cfe4d1be4d7092c5900f9d79b
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5644,IMPALA-5810: Min reservation improvements

Posted by "Matthew Jacobs (Code Review)" <ge...@cloudera.org>.

Matthew Jacobs has posted comments on this change.

Change subject: IMPALA-5644,IMPALA-5810: Min reservation improvements
......................................................................

Patch Set 4:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/7678/5/be/src/runtime/bufferpool/reservation-util.h
File be/src/runtime/bufferpool/reservation-util.h:

PS5, Line 32: allocated to buffer reservations,
> After thinking about these variables more, this paragraph doesn't seem righ
I think it's the latter.

Maybe it'd be more clear if the first sentence was changed to:

The fraction of the query mem limit that is used as the maximum buffer reservation limit. It is expected that memory not accounted by buffer reservation trackers stays within (1 - RESERVATION_MEM_FRACTION).

I think the part of the text that you highlighted is just explaining the motivation for having a high value. What do you/Tim think?

PS5, Line 36:
: /// The limit on reservations is co
> I don't follow that. we don't use buffer reservations for those yet.
I think this is providing examples of things that use the memory that is left after buffer reservations. I agree that depending on how you read it, it could be misleading. I can reword this to try to be more clear.

I incorporated this text from Tim's change to reduce RESERVATION_MEM_MIN_REMAINING, so it'd be good for him to comment as well.

Tim, is there any comment anywhere else about the 'classes' of memory already?

http://gerrit.cloudera.org:8080/#/c/7678/4/be/src/scheduling/admission-controller.cc
File be/src/scheduling/admission-controller.cc:

Line 125: "more information.";
> I think we should soon make it so you can't disable admission control. Not
Ok, I filed IMPALA-5814 for that.

PS4, Line 429: GetBufferReservationLimitFromMemLimit
> I missed that. But should line 425 be an else-if then?
This was actually my intention originally, and that's why I asked the question that I did in query-state.cc. I think it makes sense to apply both limits, but if we won't do that in query-state.cc then I'll make this code match.

--
To view, visit http://gerrit.cloudera.org:8080/7678
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

[Impala-ASF-CR] IMPALA-5644: Reject queries if min reservation is too large

Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.

Tim Armstrong has posted comments on this change.

Change subject: IMPALA-5644: Reject queries if min reservation is too large
......................................................................


Patch Set 1:

(4 comments)

The overall approach makes sense to me. Commented on the aspects of it that weren't covered by the TODOs that you left.

http://gerrit.cloudera.org:8080/#/c/7678/1/be/src/runtime/bufferpool/reservation-util.cc
File be/src/runtime/bufferpool/reservation-util.cc:

Line 23:   const double ReservationUtil::RESERVATION_MEM_FRACTION = 0.8;
We don't normally indent inside namespaces.


http://gerrit.cloudera.org:8080/#/c/7678/1/be/src/runtime/query-state.cc
File be/src/runtime/query-state.cc:

Line 153:   if (query_options().__isset.buffer_pool_limit
The logic in admission control should also take into account buffer_pool_limit. I think that may break the approach used in test_initial_reservation() to test failure to acquire initial reservation, but I have an alternative test here that would test the initial reservation exceeding the process limit:

https://gerrit.cloudera.org/#/c/7668/


http://gerrit.cloudera.org:8080/#/c/7678/1/be/src/scheduling/admission-controller.cc
File be/src/scheduling/admission-controller.cc:

Line 428:   // Checks related to pool max_mem_resources:
Is there a reason behind the order of the checks? Might be helpful to document.


PS1, Line 445: GetProcMemLimit
Should we also be comparing the reservation against the process memory limit?


-- 
To view, visit http://gerrit.cloudera.org:8080/7678
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Iabe87ce8f460356cfe4d1be4d7092c5900f9d79b
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5644: Reject queries if min reservation is too large

Posted by "Matthew Jacobs (Code Review)" <ge...@cloudera.org>.

Hello Tim Armstrong,

I'd like you to reexamine a change.  Please visit

    http://gerrit.cloudera.org:8080/7678

to look at the new patch set (#4).

Change subject: IMPALA-5644: Reject queries if min reservation is too large
......................................................................

IMPALA-5644: Reject queries if min reservation is too large

Rejects queries during admission control if:
* the largest (across all backends) min buffer reservation is
  greater than the query mem_limit or buffer_pool_limit
* the sum of the min buffer reservations across the cluster
  is larger than the pool max mem resources

There are some other interesting cases to consider later:
* every per-backend min buffer reservation is less than the
  associated backend's process mem_limit; the current
  admission control code doesn't know about other backend's
  proc mem_limits.

Some minor code refactoring in admission-controller and
query-state.

Testing:
* Added new test cases in test_admission_controller.py

Change-Id: Iabe87ce8f460356cfe4d1be4d7092c5900f9d79b
---
M be/src/runtime/bufferpool/CMakeLists.txt
M be/src/runtime/bufferpool/reservation-tracker-test.cc
A be/src/runtime/bufferpool/reservation-util.cc
A be/src/runtime/bufferpool/reservation-util.h
M be/src/runtime/query-state.cc
M be/src/scheduling/admission-controller.cc
M be/src/scheduling/admission-controller.h
M be/src/scheduling/scheduler.h
M common/thrift/generate_error_codes.py
M tests/custom_cluster/test_admission_controller.py
10 files changed, 282 insertions(+), 64 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/78/7678/4
-- 
To view, visit http://gerrit.cloudera.org:8080/7678
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iabe87ce8f460356cfe4d1be4d7092c5900f9d79b
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>

[Impala-ASF-CR] IMPALA-5644,IMPALA-5810: Min reservation improvements

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.

Impala Public Jenkins has submitted this change and it was merged.

Change subject: IMPALA-5644,IMPALA-5810: Min reservation improvements
......................................................................


IMPALA-5644,IMPALA-5810: Min reservation improvements

Rejects queries during admission control if:
* the largest (across all backends) min buffer reservation is
  greater than the query mem_limit or buffer_pool_limit
* the sum of the min buffer reservations across the cluster
  is larger than the pool max mem resources

There are some other interesting cases to consider later:
* every per-backend min buffer reservation is less than the
  associated backend's process mem_limit; the current
  admission control code doesn't know about other backend's
  proc mem_limits.

Also reduces minimum non-reservation memory (IMPALA-5810).
See the JIRA for experimental results that show this
slightly improves min memory requirements for small queries.
One reason to tweak this is to compensate for the fact that
BufferedBlockMgr didn't count small buffers against the
BlockMgr limit, but BufferPool counts all buffers against
it.

Testing:
* Adds new test cases in test_admission_controller.py
* Adds BE tests in reservation-tracker-test for the
  reservation-util code.

Change-Id: Iabe87ce8f460356cfe4d1be4d7092c5900f9d79b
Reviewed-on: http://gerrit.cloudera.org:8080/7678
Reviewed-by: Matthew Jacobs <mj...@cloudera.com>
Tested-by: Impala Public Jenkins
---
M be/src/runtime/bufferpool/CMakeLists.txt
M be/src/runtime/bufferpool/reservation-tracker-test.cc
A be/src/runtime/bufferpool/reservation-util.cc
A be/src/runtime/bufferpool/reservation-util.h
M be/src/runtime/query-state.cc
M be/src/scheduling/admission-controller.cc
M be/src/scheduling/admission-controller.h
M be/src/scheduling/scheduler.h
M common/thrift/generate_error_codes.py
M testdata/workloads/functional-query/queries/QueryTest/analytic-fns.test
M testdata/workloads/functional-query/queries/QueryTest/spilling-aggs.test
M tests/custom_cluster/test_admission_controller.py
M tests/query_test/test_mem_usage_scaling.py
13 files changed, 298 insertions(+), 104 deletions(-)

Approvals:
  Impala Public Jenkins: Verified
  Matthew Jacobs: Looks good to me, approved



-- 
To view, visit http://gerrit.cloudera.org:8080/7678
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: merged
Gerrit-Change-Id: Iabe87ce8f460356cfe4d1be4d7092c5900f9d79b
Gerrit-PatchSet: 11
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>

[Impala-ASF-CR] IMPALA-5644,IMPALA-5810: Min reservation improvements

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.

Impala Public Jenkins has posted comments on this change.

Change subject: IMPALA-5644,IMPALA-5810: Min reservation improvements
......................................................................


Patch Set 7:

Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/1097/

-- 
To view, visit http://gerrit.cloudera.org:8080/7678
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Iabe87ce8f460356cfe4d1be4d7092c5900f9d79b
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No