Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/17683 )
Change subject: IMPALA-8762: Track host level admission stats across all coordinators ...................................................................... Patch Set 2: (8 comments) http://gerrit.cloudera.org:8080/#/c/17683/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/17683/1//COMMIT_MSG@7 PS1, Line 7: coordinators > nit: coordinators Done http://gerrit.cloudera.org:8080/#/c/17683/1/be/src/scheduling/admission-controller.h File be/src/scheduling/admission-controller.h: http://gerrit.cloudera.org:8080/#/c/17683/1/be/src/scheduling/admission-controller.h@440 PS1, Line 440: // This maps a backends's id(host/port id) to its host level statistics which are used > Nit: Its not a struct any more Done http://gerrit.cloudera.org:8080/#/c/17683/1/be/src/scheduling/admission-controller.h@442 PS1, Line 442: kends. > nit: what's the map key? host_id? Done http://gerrit.cloudera.org:8080/#/c/17683/1/be/src/scheduling/admission-controller.cc File be/src/scheduling/admission-controller.cc: http://gerrit.cloudera.org:8080/#/c/17683/1/be/src/scheduling/admission-controller.cc@75 PS1, Line 75: // "<prefix><pool_name><delimiter><backend_id>". "!" is used because the backend id > Nit: is it clearer to say it is of the form? Done http://gerrit.cloudera.org:8080/#/c/17683/1/be/src/scheduling/admission-controller.cc@244 PS1, Line 244: // Parses the topic key to separate the prefix that helps recognize the kind of update > Nit: separate Done http://gerrit.cloudera.org:8080/#/c/17683/1/be/src/scheduling/admission-controller.cc@245 PS1, Line 245: // received. > Nit: received Done http://gerrit.cloudera.org:8080/#/c/17683/1/be/src/scheduling/admission-controller.cc@1652 PS1, Line 1652: if (topic_backend_id == host_id_) continue; > This is skipping updates about the current host? this skips the update that was added by the current host in its previous statestore update, since the current host's local version is more up to date. Similar to what we do in L1635 above http://gerrit.cloudera.org:8080/#/c/17683/1/tests/custom_cluster/test_executor_groups.py File tests/custom_cluster/test_executor_groups.py: http://gerrit.cloudera.org:8080/#/c/17683/1/tests/custom_cluster/test_executor_groups.py@96 PS1, Line 96: self.num_impalads = 2 > why is this 2? this is a mis-nomer, it basically refers to the num of impalad instances that self._start_impala_cluster will look for. See L66 and L80. I'll update the naming of the variables to represent their true meaning. -- To view, visit http://gerrit.cloudera.org:8080/17683 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2946832e0a89b077d0f3bec755e4672be2088243 Gerrit-Change-Number: 17683 Gerrit-PatchSet: 2 Gerrit-Owner: Bikramjeet Vig <bikramjeet....@cloudera.com> Gerrit-Reviewer: Andrew Sherman <asher...@cloudera.com> Gerrit-Reviewer: Bikramjeet Vig <bikramjeet....@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Joe McDonnell <joemcdonn...@cloudera.com> Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com> Gerrit-Comment-Date: Tue, 20 Jul 2021 20:50:27 +0000 Gerrit-HasComments: Yes
