[Impala-ASF-CR] IMPALA-13083: Clarify REASON MEM LIMIT TOO LOW FOR RESERVATION

2024-05-23 Thread Riza Suminto (Code Review)
Riza Suminto has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/21436 )

Change subject: IMPALA-13083: Clarify REASON_MEM_LIMIT_TOO_LOW_FOR_RESERVATION
..


Patch Set 5:

Thank you Andrew for your review!


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1ef7fb7e7a194b2036c2948639a06c392590bf66
Gerrit-Change-Number: 21436
Gerrit-PatchSet: 5
Gerrit-Owner: Riza Suminto 
Gerrit-Reviewer: Abhishek Rawat 
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Riza Suminto 
Gerrit-Comment-Date: Fri, 24 May 2024 00:19:18 +
Gerrit-HasComments: No


[Impala-ASF-CR] IMPALA-13083: Clarify REASON MEM LIMIT TOO LOW FOR RESERVATION

2024-05-22 Thread Impala Public Jenkins (Code Review)
Impala Public Jenkins has submitted this change and it was merged. ( 
http://gerrit.cloudera.org:8080/21436 )

Change subject: IMPALA-13083: Clarify REASON_MEM_LIMIT_TOO_LOW_FOR_RESERVATION
..

IMPALA-13083: Clarify REASON_MEM_LIMIT_TOO_LOW_FOR_RESERVATION

This patch improves REASON_MEM_LIMIT_TOO_LOW_FOR_RESERVATION error
message by saying the specific configuration that must be adjusted such
that the query can pass the Admission Control. New fields
'per_backend_mem_to_admit_source' and
'coord_backend_mem_to_admit_source' of type MemLimitSourcePB are added
into QuerySchedulePB. These fields explain what limiting factor drives
final numbers at 'per_backend_mem_to_admit' and
'coord_backend_mem_to_admit' respectively. In turn, Admission Control
will use this information to compose a more informative error message
that the user can act upon. The new error message pattern also
explicitly mentions "Per Host Min Memory Reservation" as a place to look
at to investigate memory reservations scheduled for each backend node.

Updated documentation with examples of query rejection by Admission
Control and how to read the error message.

Testing:
- Add BE tests at admission-controller-test.cc
- Adjust and pass affected EE tests

Change-Id: I1ef7fb7e7a194b2036c2948639a06c392590bf66
Reviewed-on: http://gerrit.cloudera.org:8080/21436
Reviewed-by: Impala Public Jenkins 
Tested-by: Impala Public Jenkins 
---
M be/src/scheduling/admission-controller-test.cc
M be/src/scheduling/admission-controller.cc
M be/src/scheduling/schedule-state.cc
M be/src/scheduling/schedule-state.h
M be/src/scheduling/scheduler.cc
M be/src/scheduling/scheduler.h
M common/protobuf/admission_control_service.proto
M docs/topics/impala_admission.xml
M docs/topics/impala_mem_limit.xml
M 
testdata/workloads/functional-query/queries/QueryTest/admission-max-min-mem-limits.test
M 
testdata/workloads/functional-query/queries/QueryTest/admission-reject-min-reservation.test
M 
testdata/workloads/functional-query/queries/QueryTest/runtime_row_filter_reservations.test
12 files changed, 689 insertions(+), 127 deletions(-)

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

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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I1ef7fb7e7a194b2036c2948639a06c392590bf66
Gerrit-Change-Number: 21436
Gerrit-PatchSet: 5
Gerrit-Owner: Riza Suminto 
Gerrit-Reviewer: Abhishek Rawat 
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Riza Suminto 


[Impala-ASF-CR] IMPALA-13083: Clarify REASON MEM LIMIT TOO LOW FOR RESERVATION

2024-05-22 Thread Impala Public Jenkins (Code Review)
Impala Public Jenkins has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/21436 )

Change subject: IMPALA-13083: Clarify REASON_MEM_LIMIT_TOO_LOW_FOR_RESERVATION
..


Patch Set 4: Verified+1


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1ef7fb7e7a194b2036c2948639a06c392590bf66
Gerrit-Change-Number: 21436
Gerrit-PatchSet: 4
Gerrit-Owner: Riza Suminto 
Gerrit-Reviewer: Abhishek Rawat 
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Riza Suminto 
Gerrit-Comment-Date: Thu, 23 May 2024 03:53:59 +
Gerrit-HasComments: No


[Impala-ASF-CR] IMPALA-13083: Clarify REASON MEM LIMIT TOO LOW FOR RESERVATION

2024-05-22 Thread Riza Suminto (Code Review)
Riza Suminto has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/21436 )

Change subject: IMPALA-13083: Clarify REASON_MEM_LIMIT_TOO_LOW_FOR_RESERVATION
..


Patch Set 3:

> Patch Set 3: Verified-1
>
> Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/10660/

Hit data loading issue at ubuntu-20.04-from-scratch/2654/
Will try rerun precommit job.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1ef7fb7e7a194b2036c2948639a06c392590bf66
Gerrit-Change-Number: 21436
Gerrit-PatchSet: 3
Gerrit-Owner: Riza Suminto 
Gerrit-Reviewer: Abhishek Rawat 
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Riza Suminto 
Gerrit-Comment-Date: Wed, 22 May 2024 22:52:31 +
Gerrit-HasComments: No


[Impala-ASF-CR] IMPALA-13083: Clarify REASON MEM LIMIT TOO LOW FOR RESERVATION

2024-05-22 Thread Impala Public Jenkins (Code Review)
Impala Public Jenkins has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/21436 )

Change subject: IMPALA-13083: Clarify REASON_MEM_LIMIT_TOO_LOW_FOR_RESERVATION
..


Patch Set 4:

Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/10661/ 
DRY_RUN=false


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1ef7fb7e7a194b2036c2948639a06c392590bf66
Gerrit-Change-Number: 21436
Gerrit-PatchSet: 4
Gerrit-Owner: Riza Suminto 
Gerrit-Reviewer: Abhishek Rawat 
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Riza Suminto 
Gerrit-Comment-Date: Wed, 22 May 2024 22:52:57 +
Gerrit-HasComments: No


[Impala-ASF-CR] IMPALA-13083: Clarify REASON MEM LIMIT TOO LOW FOR RESERVATION

2024-05-22 Thread Impala Public Jenkins (Code Review)
Impala Public Jenkins has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/21436 )

Change subject: IMPALA-13083: Clarify REASON_MEM_LIMIT_TOO_LOW_FOR_RESERVATION
..


Patch Set 4: Code-Review+2


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1ef7fb7e7a194b2036c2948639a06c392590bf66
Gerrit-Change-Number: 21436
Gerrit-PatchSet: 4
Gerrit-Owner: Riza Suminto 
Gerrit-Reviewer: Abhishek Rawat 
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Riza Suminto 
Gerrit-Comment-Date: Wed, 22 May 2024 22:52:56 +
Gerrit-HasComments: No


[Impala-ASF-CR] IMPALA-13083: Clarify REASON MEM LIMIT TOO LOW FOR RESERVATION

2024-05-22 Thread Impala Public Jenkins (Code Review)
Impala Public Jenkins has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/21436 )

Change subject: IMPALA-13083: Clarify REASON_MEM_LIMIT_TOO_LOW_FOR_RESERVATION
..


Patch Set 3: Verified-1

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


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1ef7fb7e7a194b2036c2948639a06c392590bf66
Gerrit-Change-Number: 21436
Gerrit-PatchSet: 3
Gerrit-Owner: Riza Suminto 
Gerrit-Reviewer: Abhishek Rawat 
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Riza Suminto 
Gerrit-Comment-Date: Wed, 22 May 2024 22:37:41 +
Gerrit-HasComments: No


[Impala-ASF-CR] IMPALA-13083: Clarify REASON MEM LIMIT TOO LOW FOR RESERVATION

2024-05-22 Thread Impala Public Jenkins (Code Review)
Impala Public Jenkins has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/21436 )

Change subject: IMPALA-13083: Clarify REASON_MEM_LIMIT_TOO_LOW_FOR_RESERVATION
..


Patch Set 3:

Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/10660/ 
DRY_RUN=false


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1ef7fb7e7a194b2036c2948639a06c392590bf66
Gerrit-Change-Number: 21436
Gerrit-PatchSet: 3
Gerrit-Owner: Riza Suminto 
Gerrit-Reviewer: Abhishek Rawat 
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Riza Suminto 
Gerrit-Comment-Date: Wed, 22 May 2024 18:08:21 +
Gerrit-HasComments: No


[Impala-ASF-CR] IMPALA-13083: Clarify REASON MEM LIMIT TOO LOW FOR RESERVATION

2024-05-22 Thread Impala Public Jenkins (Code Review)
Impala Public Jenkins has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/21436 )

Change subject: IMPALA-13083: Clarify REASON_MEM_LIMIT_TOO_LOW_FOR_RESERVATION
..


Patch Set 3: Code-Review+2


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1ef7fb7e7a194b2036c2948639a06c392590bf66
Gerrit-Change-Number: 21436
Gerrit-PatchSet: 3
Gerrit-Owner: Riza Suminto 
Gerrit-Reviewer: Abhishek Rawat 
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Riza Suminto 
Gerrit-Comment-Date: Wed, 22 May 2024 18:08:20 +
Gerrit-HasComments: No


[Impala-ASF-CR] IMPALA-13083: Clarify REASON MEM LIMIT TOO LOW FOR RESERVATION

2024-05-22 Thread Andrew Sherman (Code Review)
Andrew Sherman has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/21436 )

Change subject: IMPALA-13083: Clarify REASON_MEM_LIMIT_TOO_LOW_FOR_RESERVATION
..


Patch Set 2: Code-Review+2

Thanks for this significant improvement


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1ef7fb7e7a194b2036c2948639a06c392590bf66
Gerrit-Change-Number: 21436
Gerrit-PatchSet: 2
Gerrit-Owner: Riza Suminto 
Gerrit-Reviewer: Abhishek Rawat 
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Riza Suminto 
Gerrit-Comment-Date: Wed, 22 May 2024 17:55:34 +
Gerrit-HasComments: No


[Impala-ASF-CR] IMPALA-13083: Clarify REASON MEM LIMIT TOO LOW FOR RESERVATION

2024-05-20 Thread Riza Suminto (Code Review)
Riza Suminto has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/21436 )

Change subject: IMPALA-13083: Clarify REASON_MEM_LIMIT_TOO_LOW_FOR_RESERVATION
..


Patch Set 2:

(15 comments)

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

http://gerrit.cloudera.org:8080/#/c/21436/1/be/src/scheduling/admission-controller-test.cc@422
PS1, Line 422: an
> Nit: and
Done


http://gerrit.cloudera.org:8080/#/c/21436/1/be/src/scheduling/admission-controller-test.cc@443
PS1, Line 443: no
> Nit: nor
Done


http://gerrit.cloudera.org:8080/#/c/21436/1/be/src/scheduling/admission-controller-test.cc@1281
PS1, Line 1281:
> Is this comment right as we don't call __set_clamp_mem_limit_query_option()
clamp_mem_limit_query_option actually default to True. I removed all 
pool_config.__set_clamp_mem_limit_query_option(true); and add test 
DedicatedCoordAdmissionDisabledPoolMemLimit and 
DedicatedCoordAdmissionIgnoreMemClamp where 
pool_config.__set_clamp_mem_limit_query_option(false).


http://gerrit.cloudera.org:8080/#/c/21436/1/be/src/scheduling/admission-controller-test.cc@1357
PS1, Line 1357:   string not_admitted_reason = "--not set--";
> Nit: add ASSERT_NE(nullptr, schedule_state);
Done


http://gerrit.cloudera.org:8080/#/c/21436/1/be/src/scheduling/admission-controller-test.cc@1386
PS1, Line 1386:   pool_config.__set_min_query_mem_limit(MEGABYTE);
> Nit: add ASSERT_NE(nullptr, schedule_state);
Done


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

http://gerrit.cloudera.org:8080/#/c/21436/1/be/src/scheduling/admission-controller.cc@206
PS1, Line 206: in
> "in the" might be clearer
Done


http://gerrit.cloudera.org:8080/#/c/21436/1/be/src/scheduling/admission-controller.cc@211
PS1, Line 211: in
> "in the" might be clearer
Done


http://gerrit.cloudera.org:8080/#/c/21436/1/be/src/scheduling/admission-controller.cc@220
PS1, Line 220: in
> "in the" might be clearer
Done


http://gerrit.cloudera.org:8080/#/c/21436/1/be/src/scheduling/schedule-state.h
File be/src/scheduling/schedule-state.h:

http://gerrit.cloudera.org:8080/#/c/21436/1/be/src/scheduling/schedule-state.h@375
PS1, Line 375:   /// Helper functions to update either
> Maybe have short comments for these functions.
Done


http://gerrit.cloudera.org:8080/#/c/21436/1/common/protobuf/admission_control_service.proto
File common/protobuf/admission_control_service.proto:

http://gerrit.cloudera.org:8080/#/c/21436/1/common/protobuf/admission_control_service.proto@207
PS1, Line 207: coord_backend_mem_to_ad
> coord_backend_mem_to_admit?
Done


http://gerrit.cloudera.org:8080/#/c/21436/1/docs/topics/impala_admission.xml
File docs/topics/impala_admission.xml:

http://gerrit.cloudera.org:8080/#/c/21436/1/docs/topics/impala_admission.xml@286
PS1, Line 286: is se
> Nit: "is set to"
Done


http://gerrit.cloudera.org:8080/#/c/21436/1/docs/topics/impala_admission.xml@309
PS1, Line 309: recommend
> Nit: can you fix this to "recommend" while you are here?
Done


http://gerrit.cloudera.org:8080/#/c/21436/1/docs/topics/impala_admission.xml@336
PS1, Line 336: E
> There is some weird character between "MEM_LIMIT" and "query option"
Should be fixed.


http://gerrit.cloudera.org:8080/#/c/21436/1/docs/topics/impala_admission.xml@353
PS1, Line 353:
> Nit: another weird character here
Should be fixed.


http://gerrit.cloudera.org:8080/#/c/21436/1/docs/topics/impala_admission.xml@357
PS1, Line 357:  nodes.
> Nit: contains
Done.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1ef7fb7e7a194b2036c2948639a06c392590bf66
Gerrit-Change-Number: 21436
Gerrit-PatchSet: 2
Gerrit-Owner: Riza Suminto 
Gerrit-Reviewer: Abhishek Rawat 
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Riza Suminto 
Gerrit-Comment-Date: Mon, 20 May 2024 22:12:19 +
Gerrit-HasComments: Yes


[Impala-ASF-CR] IMPALA-13083: Clarify REASON MEM LIMIT TOO LOW FOR RESERVATION

2024-05-20 Thread Riza Suminto (Code Review)
Hello Andrew Sherman, Abhishek Rawat, Impala Public Jenkins,

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

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

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

Change subject: IMPALA-13083: Clarify REASON_MEM_LIMIT_TOO_LOW_FOR_RESERVATION
..

IMPALA-13083: Clarify REASON_MEM_LIMIT_TOO_LOW_FOR_RESERVATION

This patch improves REASON_MEM_LIMIT_TOO_LOW_FOR_RESERVATION error
message by saying the specific configuration that must be adjusted such
that the query can pass the Admission Control. New fields
'per_backend_mem_to_admit_source' and
'coord_backend_mem_to_admit_source' of type MemLimitSourcePB are added
into QuerySchedulePB. These fields explain what limiting factor drives
final numbers at 'per_backend_mem_to_admit' and
'coord_backend_mem_to_admit' respectively. In turn, Admission Control
will use this information to compose a more informative error message
that the user can act upon. The new error message pattern also
explicitly mentions "Per Host Min Memory Reservation" as a place to look
at to investigate memory reservations scheduled for each backend node.

Updated documentation with examples of query rejection by Admission
Control and how to read the error message.

Testing:
- Add BE tests at admission-controller-test.cc
- Adjust and pass affected EE tests

Change-Id: I1ef7fb7e7a194b2036c2948639a06c392590bf66
---
M be/src/scheduling/admission-controller-test.cc
M be/src/scheduling/admission-controller.cc
M be/src/scheduling/schedule-state.cc
M be/src/scheduling/schedule-state.h
M be/src/scheduling/scheduler.cc
M be/src/scheduling/scheduler.h
M common/protobuf/admission_control_service.proto
M docs/topics/impala_admission.xml
M docs/topics/impala_mem_limit.xml
M 
testdata/workloads/functional-query/queries/QueryTest/admission-max-min-mem-limits.test
M 
testdata/workloads/functional-query/queries/QueryTest/admission-reject-min-reservation.test
M 
testdata/workloads/functional-query/queries/QueryTest/runtime_row_filter_reservations.test
12 files changed, 689 insertions(+), 127 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I1ef7fb7e7a194b2036c2948639a06c392590bf66
Gerrit-Change-Number: 21436
Gerrit-PatchSet: 2
Gerrit-Owner: Riza Suminto 
Gerrit-Reviewer: Abhishek Rawat 
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Riza Suminto 


[Impala-ASF-CR] IMPALA-13083: Clarify REASON MEM LIMIT TOO LOW FOR RESERVATION

2024-05-17 Thread Andrew Sherman (Code Review)
Andrew Sherman has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/21436 )

Change subject: IMPALA-13083: Clarify REASON_MEM_LIMIT_TOO_LOW_FOR_RESERVATION
..


Patch Set 1:

(15 comments)

I like this change a lot, it makes AC more understandable adds many new test 
cases. I have only nits and small cleanups to suggest.

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

http://gerrit.cloudera.org:8080/#/c/21436/1/be/src/scheduling/admission-controller-test.cc@422
PS1, Line 422: or
Nit: and


http://gerrit.cloudera.org:8080/#/c/21436/1/be/src/scheduling/admission-controller-test.cc@443
PS1, Line 443: or
Nit: nor


http://gerrit.cloudera.org:8080/#/c/21436/1/be/src/scheduling/admission-controller-test.cc@1281
PS1, Line 1281: pool's mem limit clamp
Is this comment right as we don't call __set_clamp_mem_limit_query_option() ?


http://gerrit.cloudera.org:8080/#/c/21436/1/be/src/scheduling/admission-controller-test.cc@1357
PS1, Line 1357:   schedule_state->set_largest_min_reservation(4 * GIGABYTE);
Nit: add ASSERT_NE(nullptr, schedule_state);


http://gerrit.cloudera.org:8080/#/c/21436/1/be/src/scheduling/admission-controller-test.cc@1386
PS1, Line 1386:   schedule_state->set_largest_min_reservation(600 * MEGABYTE);
Nit: add ASSERT_NE(nullptr, schedule_state);


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

http://gerrit.cloudera.org:8080/#/c/21436/1/be/src/scheduling/admission-controller.cc@206
PS1, Line 206: at
"in the" might be clearer


http://gerrit.cloudera.org:8080/#/c/21436/1/be/src/scheduling/admission-controller.cc@211
PS1, Line 211: at
"in the" might be clearer


http://gerrit.cloudera.org:8080/#/c/21436/1/be/src/scheduling/admission-controller.cc@220
PS1, Line 220: at
"in the" might be clearer


http://gerrit.cloudera.org:8080/#/c/21436/1/be/src/scheduling/schedule-state.h
File be/src/scheduling/schedule-state.h:

http://gerrit.cloudera.org:8080/#/c/21436/1/be/src/scheduling/schedule-state.h@375
PS1, Line 375:   void CompareMaxBackendMemToAdmit(
Maybe have short comments for these functions.


http://gerrit.cloudera.org:8080/#/c/21436/1/common/protobuf/admission_control_service.proto
File common/protobuf/admission_control_service.proto:

http://gerrit.cloudera.org:8080/#/c/21436/1/common/protobuf/admission_control_service.proto@207
PS1, Line 207: coord_backend_mem_limit
coord_backend_mem_to_admit?


http://gerrit.cloudera.org:8080/#/c/21436/1/docs/topics/impala_admission.xml
File docs/topics/impala_admission.xml:

http://gerrit.cloudera.org:8080/#/c/21436/1/docs/topics/impala_admission.xml@286
PS1, Line 286: is to
Nit: "is set to"


http://gerrit.cloudera.org:8080/#/c/21436/1/docs/topics/impala_admission.xml@309
PS1, Line 309: recommends
Nit: can you fix this to "recommend" while you are here?


http://gerrit.cloudera.org:8080/#/c/21436/1/docs/topics/impala_admission.xml@336
PS1, Line 336:  
There is some weird character between "MEM_LIMIT" and "query option"


http://gerrit.cloudera.org:8080/#/c/21436/1/docs/topics/impala_admission.xml@353
PS1, Line 353:  
Nit: another weird character here


http://gerrit.cloudera.org:8080/#/c/21436/1/docs/topics/impala_admission.xml@357
PS1, Line 357: mentions
Nit: contains



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1ef7fb7e7a194b2036c2948639a06c392590bf66
Gerrit-Change-Number: 21436
Gerrit-PatchSet: 1
Gerrit-Owner: Riza Suminto 
Gerrit-Reviewer: Abhishek Rawat 
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Riza Suminto 
Gerrit-Comment-Date: Sat, 18 May 2024 00:40:32 +
Gerrit-HasComments: Yes


[Impala-ASF-CR] IMPALA-13083: Clarify REASON MEM LIMIT TOO LOW FOR RESERVATION

2024-05-16 Thread Riza Suminto (Code Review)
Riza Suminto has uploaded this change for review. ( 
http://gerrit.cloudera.org:8080/21436


Change subject: IMPALA-13083: Clarify REASON_MEM_LIMIT_TOO_LOW_FOR_RESERVATION
..

IMPALA-13083: Clarify REASON_MEM_LIMIT_TOO_LOW_FOR_RESERVATION

This patch improves REASON_MEM_LIMIT_TOO_LOW_FOR_RESERVATION error
message by saying the specific configuration that must be adjusted such
that the query can pass the Admission Control. New fields
'per_backend_mem_to_admit_source' and
'coord_backend_mem_to_admit_source' of type MemLimitSourcePB are added
into QuerySchedulePB. These fields explain what limiting factor drives
final numbers at 'per_backend_mem_to_admit' and
'coord_backend_mem_to_admit' respectively. In turn, Admission Control
will use this information to compose a more informative error message
that the user can act upon. The new error message pattern also
explicitly mentions "Per Host Min Memory Reservation" as a place to look
at to investigate memory reservations scheduled for each backend node.

Updated documentation with examples of query rejection by Admission
Control and how to read the error message.

Testing:
- Add BE tests at admission-controller-test.cc
- Adjust and pass affected EE tests

Change-Id: I1ef7fb7e7a194b2036c2948639a06c392590bf66
---
M be/src/scheduling/admission-controller-test.cc
M be/src/scheduling/admission-controller.cc
M be/src/scheduling/schedule-state.cc
M be/src/scheduling/schedule-state.h
M be/src/scheduling/scheduler.cc
M be/src/scheduling/scheduler.h
M common/protobuf/admission_control_service.proto
M docs/topics/impala_admission.xml
M docs/topics/impala_mem_limit.xml
M 
testdata/workloads/functional-query/queries/QueryTest/admission-max-min-mem-limits.test
M 
testdata/workloads/functional-query/queries/QueryTest/admission-reject-min-reservation.test
M 
testdata/workloads/functional-query/queries/QueryTest/runtime_row_filter_reservations.test
12 files changed, 649 insertions(+), 126 deletions(-)



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I1ef7fb7e7a194b2036c2948639a06c392590bf66
Gerrit-Change-Number: 21436
Gerrit-PatchSet: 1
Gerrit-Owner: Riza Suminto