Sahil Takiar 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 stor Done 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 Done 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 Done 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 t Skipped since exec_request_ is no longer a pointer. 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 membe Changed back to a plain member. 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? | Mention thread safety concerns here, too? Done | 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? We could store plain pointers. I think the ImpalaServer still has to use shared_ptr because the ClientRequestStates can be manipulated concurrently, by multiple threads. The code comments for ClientRequestStateMap state "ClientRequestState is owned by us and referenced as a shared_ptr to allow asynchronous deletion." It might not need to be sharded, but it does need to be thread safe because RegisterQuery / UnregisterQuery need to update the map and can run in separate threads. The web UI threads read from the map as well. I could introduce a global lock to synchronize the map, but that could cause lock contention overhead similar to IMPALA-4456. 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, Done -- 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: Michael Ho <k...@cloudera.com> Gerrit-Reviewer: Sahil Takiar <stak...@cloudera.com> Gerrit-Comment-Date: Tue, 26 Nov 2019 00:50:22 +0000 Gerrit-HasComments: Yes
