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

[Impala Public Jenkins (Code Review)]

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 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

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

[Impala Public Jenkins (Code Review)]

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 
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 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Tim Armstrong 

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

[Matthew Jacobs (Code Review)]

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 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

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

[Impala Public Jenkins (Code Review)]

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 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

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

[Matthew Jacobs (Code Review)]

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 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Tim Armstrong 

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

[Impala Public Jenkins (Code Review)]

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 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

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

[Impala Public Jenkins (Code Review)]

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 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

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

[Matthew Jacobs (Code Review)]

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 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Tim Armstrong 

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

[Matthew Jacobs (Code Review)]

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 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

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

[Matthew Jacobs (Code Review)]

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 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

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

[Dan Hecht (Code Review)]

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 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

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

[Matthew Jacobs (Code Review)]

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 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

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

[Tim Armstrong (Code Review)]

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 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

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

[Matthew Jacobs (Code Review)]

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 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Tim Armstrong 

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

[Matthew Jacobs (Code Review)]

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 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

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

[Matthew Jacobs (Code Review)]

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 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Tim Armstrong 

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

[Matthew Jacobs (Code Review)]

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 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

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

[Tim Armstrong (Code Review)]

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Iabe87ce8f460356cfe4d1be4d7092c5900f9d79b
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Matthew Jacobs 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

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

[Matthew Jacobs (Code Review)]

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 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Tim Armstrong 

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

[Tim Armstrong (Code Review)]

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 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

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

[Matthew Jacobs (Code Review)]

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Iabe87ce8f460356cfe4d1be4d7092c5900f9d79b
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Matthew Jacobs 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

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

[Dan Hecht (Code Review)]

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 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes