Thomas Tauber-Marshall has posted comments on this change. ( http://gerrit.cloudera.org:8080/17181 )
Change subject: IMPALA-10518: Add ImpalaServer interface to retrieve executor membership. ...................................................................... Patch Set 3: (5 comments) http://gerrit.cloudera.org:8080/#/c/17181/3/be/src/scheduling/cluster-membership-mgr.h File be/src/scheduling/cluster-membership-mgr.h: http://gerrit.cloudera.org:8080/#/c/17181/3/be/src/scheduling/cluster-membership-mgr.h@260 PS3, Line 260: class TUpdateExecutorMembershipRequest; Its standard in Impala to put all forward declarations at the top of the file - in this case, immediately after the "namespace impala {" line http://gerrit.cloudera.org:8080/#/c/17181/3/be/src/scheduling/cluster-membership-mgr.cc File be/src/scheduling/cluster-membership-mgr.cc: http://gerrit.cloudera.org:8080/#/c/17181/3/be/src/scheduling/cluster-membership-mgr.cc@57 PS3, Line 57: impala:: You can just put this function in the "namespace impala{" block below and remove this http://gerrit.cloudera.org:8080/#/c/17181/3/be/src/service/impala-hs2-server.cc File be/src/service/impala-hs2-server.cc: http://gerrit.cloudera.org:8080/#/c/17181/3/be/src/service/impala-hs2-server.cc@1230 PS3, Line 1230: PopulateExecutorMembershipRequest(membership_snapshot, : return_val.executor_membership); nit: I think this can fit on a single line More generally, you've got a few minor formatting errors, could you run this through clang-format-diff as described here: https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=65868536 http://gerrit.cloudera.org:8080/#/c/17181/3/common/thrift/ImpalaService.thrift File common/thrift/ImpalaService.thrift: http://gerrit.cloudera.org:8080/#/c/17181/3/common/thrift/ImpalaService.thrift@870 PS3, Line 870: // Returns the executor membership information Note that this is only supported for the external fe service http://gerrit.cloudera.org:8080/#/c/17181/3/tests/hs2/test_hs2.py File tests/hs2/test_hs2.py: http://gerrit.cloudera.org:8080/#/c/17181/3/tests/hs2/test_hs2.py@750 PS3, Line 750: assert get_executor_membership_resp.executor_membership.num_executors == 3 I'm confused as to how we would get the correct value here if the response was an error, as checked in the previous line -- To view, visit http://gerrit.cloudera.org:8080/17181 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie89b71f4555c368869ee7b9d6341756c60af12b5 Gerrit-Change-Number: 17181 Gerrit-PatchSet: 3 Gerrit-Owner: Aman Sinha <amsi...@cloudera.com> Gerrit-Reviewer: Aman Sinha <amsi...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Kurt Deschler <kdesc...@cloudera.com> Gerrit-Reviewer: Thomas Tauber-Marshall <tmarsh...@cloudera.com> Gerrit-Comment-Date: Mon, 15 Mar 2021 21:07:28 +0000 Gerrit-HasComments: Yes
