Kurt Deschler 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 Done 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 basical Done 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 f Done 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 de Done 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 th Done 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 Done 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_clien Done -- 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 <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Joe McDonnell <[email protected]> Gerrit-Reviewer: John Sherman <[email protected]> Gerrit-Reviewer: Kurt Deschler <[email protected]> Gerrit-Reviewer: Thomas Tauber-Marshall <[email protected]> Gerrit-Comment-Date: Mon, 01 Mar 2021 15:37:15 +0000 Gerrit-HasComments: Yes
