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