Joe McDonnell has posted comments on this change. ( http://gerrit.cloudera.org:8080/20524 )
Change subject: IMPALA-12426: Adds the backend InternalServer class. ...................................................................... Patch Set 16: (2 comments) I'm wrapping my head around this. Here are some initial comments. http://gerrit.cloudera.org:8080/#/c/20524/16/be/src/runtime/query-driver.h File be/src/runtime/query-driver.h: http://gerrit.cloudera.org:8080/#/c/20524/16/be/src/runtime/query-driver.h@75 PS16, Line 75: /// Wraps a `std:shared_ptr<QueryHandle>` to make it easier to access the query handle : /// methods. The `QueryHandle` struct overloads the member access operator which makes : /// working with shared_ptrs very awkward. This class restores the ability to cleanly : /// use the member access operator. : class QueryHandlePtr { : private: : std::shared_ptr<QueryHandle> ptr_; : : public: : QueryHandlePtr() { : this->ptr_ = make_shared<QueryHandle>(); : } : : QueryHandlePtr(QueryHandle* new_ptr) { : this->ptr_.reset(new_ptr); : } : : ClientRequestState* operator->() const { : return this->ptr_->operator->(); : } : : QueryHandle* get() { : return this->ptr_.get(); : } : : void reset(QueryHandle* new_ptr) { : this->ptr_.reset(new_ptr); : } : }; // class QueryHandlePtr What is the shared_ptr for? As a reference-counted pointer, when would the reference count be greater than 1? I'm thinking it might be cleaner to have the internal client behave more like existing protocols and avoid using QueryHandle directly. If we define a struct that contains the query id, then the internal client can provide that as the handle for the query. It would use GetActiveQueryHandle just like Beeswax/HS2. 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@171 PS16, Line 171: virtual Status ExecuteAndWait(const std::string& user_name, : const std::string& sql, QueryHandlePtr& query_handle) = 0; Nit: I think it would be a bit cleaner to avoid having an execute function open and session and leave it open. Either the session lifetime is solely within the function (created/closed) or the session is passed in. -- 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-Comment-Date: Fri, 03 Nov 2023 23:54:47 +0000 Gerrit-HasComments: Yes
