Lars Volker has posted comments on this change. ( http://gerrit.cloudera.org:8080/14755 )
Change subject: IMPALA-9124: ImpalaServer and ClientRequestState refactoring ...................................................................... Patch Set 2: (7 comments) http://gerrit.cloudera.org:8080/#/c/14755/2/be/src/service/client-request-state-map.h File be/src/service/client-request-state-map.h: http://gerrit.cloudera.org:8080/#/c/14755/2/be/src/service/client-request-state-map.h@35 PS2, Line 35: Status AddClientRequestState( After giving this more thought I'm skeptical of the fact that this map stores shared_ptrs to non-const objects. Should we mention in the class comment that users of the map need to synchronize access to the contained elements? http://gerrit.cloudera.org:8080/#/c/14755/2/be/src/service/client-request-state.h File be/src/service/client-request-state.h: http://gerrit.cloudera.org:8080/#/c/14755/2/be/src/service/client-request-state.h@53 PS2, Line 53: /// Execution state of the client-facing side a query. This captures everything side of a query http://gerrit.cloudera.org:8080/#/c/14755/2/be/src/service/client-request-state.h@188 PS2, Line 188: It is created what is "It" here? The instance of this class, or query_ctx? Or should this say "This method is called by.."? http://gerrit.cloudera.org:8080/#/c/14755/2/be/src/service/client-request-state.h@230 PS2, Line 230: const TExecRequest* exec_request() const { return exec_request_.get(); } Could add a DCHECK != nullptr and (nit) some whitespace around the method that requires a comment (see above). The signature could also go back to const& as it was before. http://gerrit.cloudera.org:8080/#/c/14755/2/be/src/service/client-request-state.h@469 PS2, Line 469: std::unique_ptr<TExecRequest> exec_request_; It is owned by this class, is the reason it cannot go back to a plain member that its creation is deferred? We never release ownership of it at least, right? http://gerrit.cloudera.org:8080/#/c/14755/2/be/src/service/impala-http-handler.h File be/src/service/impala-http-handler.h: http://gerrit.cloudera.org:8080/#/c/14755/2/be/src/service/impala-http-handler.h@57 PS2, Line 57: /// duplicate ClientRequestStates. Mention thread safety concerns here, too? Do we actually need shared ownership, i.e. can it happen that we store a CRS here that the ImpalaServer has already released? If not, could we also store plain pointers here and unique_ptr in the ImpalaServer to simplify ownership? It looks like we only ever call DoFuncForAllEntries() on the map, so it might not even have to be sharded? http://gerrit.cloudera.org:8080/#/c/14755/2/be/src/service/impala-server.cc File be/src/service/impala-server.cc: http://gerrit.cloudera.org:8080/#/c/14755/2/be/src/service/impala-server.cc@938 PS2, Line 938: RETURN_IF_ERROR((*request_state)->CreateExecRequest(query_ctx)); I wonder if a comment here would help understand the effects of this call, something like "calls into the frontend to get an ExecRequest" or similar. I don't feel strongly about it though. -- To view, visit http://gerrit.cloudera.org:8080/14755 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib92c932a69802225af3fd9bf15f85c85317acaca Gerrit-Change-Number: 14755 Gerrit-PatchSet: 2 Gerrit-Owner: Sahil Takiar <stak...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Lars Volker <l...@cloudera.com> Gerrit-Reviewer: Sahil Takiar <stak...@cloudera.com> Gerrit-Comment-Date: Thu, 21 Nov 2019 22:33:57 +0000 Gerrit-HasComments: Yes
