Michael Smith has posted comments on this change. ( http://gerrit.cloudera.org:8080/20524 )
Change subject: IMPALA-12426: Adds the backend InternalServer class. ...................................................................... Patch Set 16: (7 comments) I haven't gone through all the implementation yet, just leaving a few thoughts here. http://gerrit.cloudera.org:8080/#/c/20524/16/be/src/service/impala-server.h File be/src/service/impala-server.h: http://gerrit.cloudera.org:8080/#/c/20524/16/be/src/service/impala-server.h@403 PS16, Line 403: /// InternalServer methods, see internal-server.h for details We don't strictly need an abstract parent for this. But as a matter of documentation it is kind of nice. http://gerrit.cloudera.org:8080/#/c/20524/16/be/src/service/impala-server.h@404 PS16, Line 404: virtual Status OpenSession(const std::string& user_name, TUniqueId& new_session_id, You don't need 2 separate functions here, you can set a default value for query_opts. Similar feedback applies to several functions here. http://gerrit.cloudera.org:8080/#/c/20524/16/be/src/service/impala-server.h@1297 PS16, Line 1297: /// helper functions for the InternalServer functions If we combine redundant function definitions above, I don't think we need these functions anymore. http://gerrit.cloudera.org:8080/#/c/20524/16/be/src/service/impala-server.cc File be/src/service/impala-server.cc: http://gerrit.cloudera.org:8080/#/c/20524/16/be/src/service/impala-server.cc@2586 PS16, Line 2586: uuid secret_uuid; Can we use RandomUniqueID here? http://gerrit.cloudera.org:8080/#/c/20524/16/be/src/service/impala-server.cc@3469 PS16, Line 3469: uuid conn_uuid; nit: indentation is off http://gerrit.cloudera.org:8080/#/c/20524/16/be/src/service/internal-server.h File be/src/service/internal-server.h: http://gerrit.cloudera.org:8080/#/c/20524/16/be/src/service/internal-server.h@45 PS16, Line 45: /// The easiest way is to use this class is to call the `ExecuteAndFetchAllText` remove first "is" http://gerrit.cloudera.org:8080/#/c/20524/16/be/src/service/internal-server.cc File be/src/service/internal-server.cc: http://gerrit.cloudera.org:8080/#/c/20524/16/be/src/service/internal-server.cc@35 PS16, Line 35: using boost::uuids::uuid; Do you use this somewhere? -- To view, visit http://gerrit.cloudera.org:8080/20524 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I27686aa563fac87429657e4980b29b0da91eb9e1 Gerrit-Change-Number: 20524 Gerrit-PatchSet: 16 Gerrit-Owner: Jason Fehr <jf...@cloudera.com> Gerrit-Reviewer: Abhishek Rawat <ara...@cloudera.com> Gerrit-Reviewer: Andrew Sherman <asher...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Jason Fehr <jf...@cloudera.com> Gerrit-Reviewer: Joe McDonnell <joemcdonn...@cloudera.com> Gerrit-Reviewer: Michael Smith <michael.sm...@cloudera.com> Gerrit-Comment-Date: Wed, 08 Nov 2023 00:32:59 +0000 Gerrit-HasComments: Yes
