Andrew Sherman has posted comments on this change. ( http://gerrit.cloudera.org:8080/21896 )
Change subject: IMPALA-12146: Fix incorrect host memory reserved when the executor quits abnormally ...................................................................... Patch Set 3: (5 comments) Thanks for the patch http://gerrit.cloudera.org:8080/#/c/21896/3/be/src/scheduling/admission-controller-test.cc File be/src/scheduling/admission-controller-test.cc: http://gerrit.cloudera.org:8080/#/c/21896/3/be/src/scheduling/admission-controller-test.cc@549 PS3, Line 549: TEST_F(AdmissionControllerTest, EraseHostStats) { Add a brief descriptive comment http://gerrit.cloudera.org:8080/#/c/21896/3/be/src/scheduling/admission-controller.cc File be/src/scheduling/admission-controller.cc: http://gerrit.cloudera.org:8080/#/c/21896/3/be/src/scheduling/admission-controller.cc@1768 PS3, Line 1768: removed_nodes.insert(topic_backend_id); The method comment says "Any remote host stats removed during the process will result in the host being added to the removed_nodes set", which happens on line 1786, but here we are also doing it for pool stat updates. So comment and code are inconsistent. I see that the unit test does test both topics. And the commit message is also confusing, initially mentioning "pool stat updates" when describing the problem, but "remote pool stats" when describing the fix. http://gerrit.cloudera.org:8080/#/c/21896/3/be/src/scheduling/admission-controller.cc@1880 PS3, Line 1880: // Reset host stats memory reserved if the corresponding remote stats were removed. Nit: I know this line is describing what we are doing but it does not describe the code. Consider just deleting this line http://gerrit.cloudera.org:8080/#/c/21896/3/be/src/scheduling/admission-controller.cc@1881 PS3, Line 1881: check Nit: maybe "know" is better, we already did thr check http://gerrit.cloudera.org:8080/#/c/21896/3/be/src/scheduling/admission-controller.cc@1892 PS3, Line 1892: Stats Maybe "mem_reserved" or similar would be clearer than just "Stats"? -- To view, visit http://gerrit.cloudera.org:8080/21896 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic6f6edd28c55904d63d0c494230ee2bf7a0f6cce Gerrit-Change-Number: 21896 Gerrit-PatchSet: 3 Gerrit-Owner: Yida Wu <[email protected]> Gerrit-Reviewer: Abhishek Rawat <[email protected]> Gerrit-Reviewer: Andrew Sherman <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Comment-Date: Tue, 08 Oct 2024 17:44:05 +0000 Gerrit-HasComments: Yes
