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 <riza.sumi...@cloudera.com>
Gerrit-Reviewer: Abhishek Rawat <ara...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <asher...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <riza.sumi...@cloudera.com>
Gerrit-Comment-Date: Mon, 20 May 2024 22:12:19 +0000
Gerrit-HasComments: Yes

Reply via email to