Thomas Tauber-Marshall has posted comments on this change. ( http://gerrit.cloudera.org:8080/17116 )
Change subject: IMPALA-10546: Add ImpalaServer interface to retrieve BackendConfig from impalad ...................................................................... Patch Set 9: (7 comments) http://gerrit.cloudera.org:8080/#/c/17116/9/be/src/rpc/hs2-http-test.cc File be/src/rpc/hs2-http-test.cc: http://gerrit.cloudera.org:8080/#/c/17116/9/be/src/rpc/hs2-http-test.cc@53 PS9, Line 53: return_val nit: we've used '_return' for all of the other functions here http://gerrit.cloudera.org:8080/#/c/17116/9/be/src/service/impala-hs2-server.cc File be/src/service/impala-hs2-server.cc: http://gerrit.cloudera.org:8080/#/c/17116/9/be/src/service/impala-hs2-server.cc@1193 PS9, Line 1193: shared_ptr<SessionState> session; I don't think the session is actually used for anything here, we're basically just checking that it exists. Any reason to not just leave 'sessionHandle' out of the request entirely and save ourselves some extra work? http://gerrit.cloudera.org:8080/#/c/17116/9/be/src/util/backend-gflag-util.h File be/src/util/backend-gflag-util.h: http://gerrit.cloudera.org:8080/#/c/17116/9/be/src/util/backend-gflag-util.h@27 PS9, Line 27: /// Builds the TBackendGflags object to pass to JNI. This is used to pass the gflag : /// configs to the Frontend and the Catalog. It would be cleaner to put this comment directly above the version of the function that it really applies to, i.e. GetThriftBackendGflags http://gerrit.cloudera.org:8080/#/c/17116/9/be/src/util/backend-gflag-util.h@29 PS9, Line 29: class TBackendGflags; Not a big deal, but its pretty standard in Impala to put all the forward declaration together at the top, i.e. in this case directly after the "namespace impala {" line above http://gerrit.cloudera.org:8080/#/c/17116/9/be/src/util/backend-gflag-util.h@31 PS9, Line 31: GetThriftBackendGflags Might be nice to rename this, eg. to GetThriftBackendGFlagsForJNI, since the difference between GetThriftBackendGflags and PopulateThriftBackendGflags isn't very clear http://gerrit.cloudera.org:8080/#/c/17116/9/common/thrift/ImpalaService.thrift File common/thrift/ImpalaService.thrift: http://gerrit.cloudera.org:8080/#/c/17116/9/common/thrift/ImpalaService.thrift@857 PS9, Line 857: // Returns the current TBackendGflags Maybe mention that this is only supported for the "external fe" server http://gerrit.cloudera.org:8080/#/c/17116/9/tests/hs2/test_hs2.py File tests/hs2/test_hs2.py: http://gerrit.cloudera.org:8080/#/c/17116/9/tests/hs2/test_hs2.py@738 PS9, Line 738: hs2_client I'm not sure how this test would work, since I would assune that 'hs2_client' here would point at the normal hs2 port, not the "external frontend" port, so shouldn't we hit the "Unsupported operation" error. Of course, in addition to the case where it works that it tested here, it would be nice to include a test that checks that it gets the error in the cases where it should. -- To view, visit http://gerrit.cloudera.org:8080/17116 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I14a3cee29f1fc91f4431b7ea89053bb3fbfa5e69 Gerrit-Change-Number: 17116 Gerrit-PatchSet: 9 Gerrit-Owner: Kurt Deschler <kdesc...@cloudera.com> Gerrit-Reviewer: Anonymous Coward <j...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Joe McDonnell <joemcdonn...@cloudera.com> Gerrit-Reviewer: Kurt Deschler <kdesc...@cloudera.com> Gerrit-Reviewer: Thomas Tauber-Marshall <tmarsh...@cloudera.com> Gerrit-Comment-Date: Fri, 26 Feb 2021 20:41:17 +0000 Gerrit-HasComments: Yes
