Jason Fehr 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) 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 docu The interface class InternalServer is nice because my next commit will introduce a variable of type InternalServer in the ExecEnv class. Coordinators can use this variable to submit queries to themselves. Having only the applicable methods available makes it much easier to know which methods can be called. 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 q Done. I had originally wanted an interface without pointers, but, after looking at it again, using pointers in the interface makes the code cleaner and the helper functions go away. 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 t Done 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? yes, done 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 Done 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" Done 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? Nope, that was leftover code. Good catch! -- 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 <[email protected]> Gerrit-Reviewer: Abhishek Rawat <[email protected]> Gerrit-Reviewer: Andrew Sherman <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Jason Fehr <[email protected]> Gerrit-Reviewer: Joe McDonnell <[email protected]> Gerrit-Reviewer: Michael Smith <[email protected]> Gerrit-Comment-Date: Wed, 08 Nov 2023 16:35:01 +0000 Gerrit-HasComments: Yes
