Aman Sinha 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 5: (7 comments) Thanks for the review. 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: }; > Its standard in Impala to put all forward declarations at the top of the fi Done 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: tring lo > You can just put this function in the "namespace impala{" block below and r Done 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: : // Populate an instance of TUpdate > nit: I think this can fit on a single line That's good to know.. I ran that tool and it certainly improved the formatting in a few places. 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. Only supported for the "external fe" > Note that this is only supported for the external fe service Done 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@746 PS3, Line 746: > flake8: E501 line too long (101 > 90 characters) Done http://gerrit.cloudera.org:8080/#/c/17181/3/tests/hs2/test_hs2.py@750 PS3, Line 750: > I'm confused as to how we would get the correct value here if the response Hmm..this line was not present in patch set 1 and 2 but appeared in PS 3 because of some local changes I was trying. It's not supposed to be there. Thanks for pointing it out. http://gerrit.cloudera.org:8080/#/c/17181/4/tests/hs2/test_hs2.py File tests/hs2/test_hs2.py: http://gerrit.cloudera.org:8080/#/c/17181/4/tests/hs2/test_hs2.py@747 PS4, Line 747: e > flake8: E126 continuation line over-indented for hanging indent Done -- 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: 5 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: Wed, 17 Mar 2021 05:46:56 +0000 Gerrit-HasComments: Yes
