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 <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: Sat, 18 May 2024 00:40:32 +0000 Gerrit-HasComments: Yes
