You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Sahil Takiar (Code Review)" <ge...@cloudera.org> on 2019/11/20 20:45:15 UTC
[Impala-ASF-CR] IMPALA-9124: ImpalaServer and ClientRequestState refactoring
Sahil Takiar has uploaded this change for review. ( http://gerrit.cloudera.org:8080/14755
Change subject: IMPALA-9124: ImpalaServer and ClientRequestState refactoring
......................................................................
IMPALA-9124: ImpalaServer and ClientRequestState refactoring
Re-factoring several areas of the ImpalaServer and ClientRequestState
in preparation for future work required for transparent query retries.
Re-factored TExecRequest ownership in ClientRequestState. Previously,
the TExecRequest was passed to the ClientRequestState via the
Exec(TExecRequest) method. Now it is passed via the constructor and its
lifetime is managed by a shared_ptr. Using a shared_ptr avoids a copy of
the TExecRequest that was happening in
ClientRequestState::Exec(TExecRequest).
Re-factored ImpalaServer::UnregisterQuery into multiple methods. Made
ClientRequestStateMap a dedicated class that extends ShardedQueryMap
with additional methods to add and delete a ClientRequestState.
Re-factored ImpalaHttpHandler's usage of the ClientRequestStateMap so
that it tracks its own list of running query ids / ClientRequestStates
rather than using the one owned by ImpalaServer. This is particularly
important for transparent query retries because retrying a query causes
duplicate ClientRequestStates to be added to the
ImpalaServer::client_request_state_map_.
Testing:
* Ran core tests
Change-Id: Ib92c932a69802225af3fd9bf15f85c85317acaca
---
M be/src/service/child-query.cc
A be/src/service/client-request-state-map.h
M be/src/service/client-request-state.cc
M be/src/service/client-request-state.h
M be/src/service/impala-hs2-server.cc
M be/src/service/impala-http-handler.cc
M be/src/service/impala-http-handler.h
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
9 files changed, 205 insertions(+), 119 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/55/14755/1
--
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: newchange
Gerrit-Change-Id: Ib92c932a69802225af3fd9bf15f85c85317acaca
Gerrit-Change-Number: 14755
Gerrit-PatchSet: 1
Gerrit-Owner: Sahil Takiar <st...@cloudera.com>
[Impala-ASF-CR] IMPALA-9124: ImpalaServer and ClientRequestState refactoring
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14755 )
Change subject: IMPALA-9124: ImpalaServer and ClientRequestState refactoring
......................................................................
Patch Set 3:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/5139/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
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: 3
Gerrit-Owner: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Comment-Date: Tue, 26 Nov 2019 01:32:36 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9124: ImpalaServer and ClientRequestState refactoring
Posted by "Sahil Takiar (Code Review)" <ge...@cloudera.org>.
Sahil Takiar has posted comments on this change. ( http://gerrit.cloudera.org:8080/14755 )
Change subject: IMPALA-9124: ImpalaServer and ClientRequestState refactoring
......................................................................
Patch Set 8:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/14755/8/be/src/service/client-request-state.h
File be/src/service/client-request-state.h:
http://gerrit.cloudera.org:8080/#/c/14755/8/be/src/service/client-request-state.h@231
PS8, Line 231: /// Contents are only valid after InitExecRequest(TQueryCtx) initializes the
> line has trailing whitespace
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: 8
Gerrit-Owner: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Comment-Date: Wed, 04 Dec 2019 03:45:49 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-9124: ImpalaServer and ClientRequestState refactoring
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14755 )
Change subject: IMPALA-9124: ImpalaServer and ClientRequestState refactoring
......................................................................
Patch Set 6:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/5188/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
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: 6
Gerrit-Owner: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Comment-Date: Wed, 04 Dec 2019 03:25:02 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9124: ImpalaServer and ClientRequestState refactoring
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14755 )
Change subject: IMPALA-9124: ImpalaServer and ClientRequestState refactoring
......................................................................
Patch Set 4:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/5294/ DRY_RUN=true
--
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: 4
Gerrit-Owner: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Comment-Date: Tue, 26 Nov 2019 01:38:26 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9124: ImpalaServer and ClientRequestState refactoring
Posted by "Sahil Takiar (Code Review)" <ge...@cloudera.org>.
Hello Michael Ho, Lars Volker, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/14755
to look at the new patch set (#7).
Change subject: IMPALA-9124: ImpalaServer and ClientRequestState refactoring
......................................................................
IMPALA-9124: ImpalaServer and ClientRequestState refactoring
Re-factoring several areas of the ImpalaServer and ClientRequestState
in preparation for future work required for transparent query retries.
Re-factored TExecRequest ownership in ClientRequestState. Previously,
the TExecRequest was passed to the ClientRequestState via the
Exec(TExecRequest) method. Now it is created and owned by the
ClientRequestState and only exposed as a constant reference. It is
stored as a plain member variable.
Re-factored ImpalaServer::UnregisterQuery into multiple methods. Made
ClientRequestStateMap a dedicated class that extends ShardedQueryMap
with additional methods to add and delete a ClientRequestState. The
re-factoring breaks up the large UnregisterQuery method into multiple
smaller method and adds some additional code documentation.
Re-factored ImpalaServer::client_request_state_map_ into a separate
class.
Testing:
* Ran core tests
Change-Id: Ib92c932a69802225af3fd9bf15f85c85317acaca
---
A be/src/service/client-request-state-map.h
M be/src/service/client-request-state.cc
M be/src/service/client-request-state.h
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
5 files changed, 149 insertions(+), 60 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/55/14755/7
--
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: newpatchset
Gerrit-Change-Id: Ib92c932a69802225af3fd9bf15f85c85317acaca
Gerrit-Change-Number: 14755
Gerrit-PatchSet: 7
Gerrit-Owner: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
[Impala-ASF-CR] IMPALA-9124: ImpalaServer and ClientRequestState refactoring
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14755 )
Change subject: IMPALA-9124: ImpalaServer and ClientRequestState refactoring
......................................................................
Patch Set 10:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/5323/ DRY_RUN=false
--
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: 10
Gerrit-Owner: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <mi...@gmail.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Comment-Date: Mon, 09 Dec 2019 15:30:48 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9124: ImpalaServer and ClientRequestState refactoring
Posted by "Lars Volker (Code Review)" <ge...@cloudera.org>.
Lars Volker has posted comments on this change. ( http://gerrit.cloudera.org:8080/14755 )
Change subject: IMPALA-9124: ImpalaServer and ClientRequestState refactoring
......................................................................
Patch Set 1:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/14755/1/be/src/service/client-request-state-map.h
File be/src/service/client-request-state-map.h:
http://gerrit.cloudera.org:8080/#/c/14755/1/be/src/service/client-request-state-map.h@35
PS1, Line 35: const std::shared_ptr<ClientRequestState>& request_state) {
We should take the shared_ptr by value since we are going to retain a copy of it. I.e., I think we should follow this advice:
> Guideline: Express that a function will store and share ownership of a heap object using a by-value shared_ptr parameter.
(https://herbsutter.com/2013/06/05/gotw-91-solution-smart-pointer-parameters/)
http://gerrit.cloudera.org:8080/#/c/14755/1/be/src/service/client-request-state-map.h@44
PS1, Line 44: ss << "query id " << PrintId(query_id) << " already exists";
This could use "Substitute()" to be a bit more concise
http://gerrit.cloudera.org:8080/#/c/14755/1/be/src/service/client-request-state-map.h@55
PS1, Line 55: Status DeleteClientRequestState(
You could add an overload without the request_state argument to make the usage cleaner. Looking at DeleteClientRequestState(state, nullptr) without any context wasn't all to clear to me.
http://gerrit.cloudera.org:8080/#/c/14755/1/be/src/service/impala-http-handler.h
File be/src/service/impala-http-handler.h:
http://gerrit.cloudera.org:8080/#/c/14755/1/be/src/service/impala-http-handler.h@55
PS1, Line 55: /// Tracks the ClientRequestStates of the currently registered queries.
Can you add to the comment why we store them here as well as in the ImpalaServer?
http://gerrit.cloudera.org:8080/#/c/14755/1/be/src/service/impala-server.h
File be/src/service/impala-server.h:
http://gerrit.cloudera.org:8080/#/c/14755/1/be/src/service/impala-server.h@1084
PS1, Line 1084: ClientRequestState is owned
What does this mean in the context of shared_ptr? If it is owned, shouldn't it be a unique_ptr?
--
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: 1
Gerrit-Owner: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Comment-Date: Wed, 20 Nov 2019 22:27:31 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-9124: ImpalaServer and ClientRequestState refactoring
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14755 )
Change subject: IMPALA-9124: ImpalaServer and ClientRequestState refactoring
......................................................................
Patch Set 11:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/5324/ DRY_RUN=false
--
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: 11
Gerrit-Owner: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <mi...@gmail.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Comment-Date: Mon, 09 Dec 2019 16:28:30 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9124: ImpalaServer and ClientRequestState refactoring
Posted by "Lars Volker (Code Review)" <ge...@cloudera.org>.
Lars Volker has posted comments on this change. ( http://gerrit.cloudera.org:8080/14755 )
Change subject: IMPALA-9124: ImpalaServer and ClientRequestState refactoring
......................................................................
Patch Set 9: Code-Review+2
(1 comment)
http://gerrit.cloudera.org:8080/#/c/14755/5/be/src/service/client-request-state.h
File be/src/service/client-request-state.h:
http://gerrit.cloudera.org:8080/#/c/14755/5/be/src/service/client-request-state.h@231
PS5, Line 231: /// Contents are only valid after InitExecRequest(TQueryCtx) initializes the
> I'm actually about to change this again in an upcoming patch, so I'm not su
Sounds good, let's defer
--
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: 9
Gerrit-Owner: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Comment-Date: Wed, 04 Dec 2019 17:51:35 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-9124: ImpalaServer and ClientRequestState refactoring
Posted by "Sahil Takiar (Code Review)" <ge...@cloudera.org>.
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 <st...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Comment-Date: Tue, 26 Nov 2019 00:50:22 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-9124: ImpalaServer and ClientRequestState refactoring
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14755 )
Change subject: IMPALA-9124: ImpalaServer and ClientRequestState refactoring
......................................................................
Patch Set 11: Verified+1
--
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: 11
Gerrit-Owner: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <mi...@gmail.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Comment-Date: Mon, 09 Dec 2019 20:52:18 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9124: ImpalaServer and ClientRequestState refactoring
Posted by "Sahil Takiar (Code Review)" <ge...@cloudera.org>.
Hello Michael Ho, Lars Volker, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/14755
to look at the new patch set (#9).
Change subject: IMPALA-9124: ImpalaServer and ClientRequestState refactoring
......................................................................
IMPALA-9124: ImpalaServer and ClientRequestState refactoring
Re-factoring several areas of the ImpalaServer and ClientRequestState
in preparation for future work required for transparent query retries.
Re-factored TExecRequest ownership in ClientRequestState. Previously,
the TExecRequest was passed to the ClientRequestState via the
Exec(TExecRequest) method. Now it is created and owned by the
ClientRequestState and only exposed as a constant reference. It is
stored as a plain member variable.
Re-factored ImpalaServer::UnregisterQuery into multiple methods. Made
ClientRequestStateMap a dedicated class that extends ShardedQueryMap
with additional methods to add and delete a ClientRequestState. The
re-factoring breaks up the large UnregisterQuery method into multiple
smaller method and adds some additional code documentation.
Re-factored ImpalaServer::client_request_state_map_ into a separate
class.
Testing:
* Ran core tests
Change-Id: Ib92c932a69802225af3fd9bf15f85c85317acaca
---
A be/src/service/client-request-state-map.h
M be/src/service/client-request-state.cc
M be/src/service/client-request-state.h
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
5 files changed, 150 insertions(+), 60 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/55/14755/9
--
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: newpatchset
Gerrit-Change-Id: Ib92c932a69802225af3fd9bf15f85c85317acaca
Gerrit-Change-Number: 14755
Gerrit-PatchSet: 9
Gerrit-Owner: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
[Impala-ASF-CR] IMPALA-9124: ImpalaServer and ClientRequestState refactoring
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14755 )
Change subject: IMPALA-9124: ImpalaServer and ClientRequestState refactoring
......................................................................
Patch Set 2:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/5103/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
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 <st...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Comment-Date: Thu, 21 Nov 2019 20:16:57 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9124: ImpalaServer and ClientRequestState refactoring
Posted by "Sahil Takiar (Code Review)" <ge...@cloudera.org>.
Sahil Takiar has posted comments on this change. ( http://gerrit.cloudera.org:8080/14755 )
Change subject: IMPALA-9124: ImpalaServer and ClientRequestState refactoring
......................................................................
Patch Set 11: Code-Review+2
Fixing build failure (forgot to rename LoadExecRequest to InitExecRequest in ImpalaServer).
Carrying +2.
--
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: 11
Gerrit-Owner: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <mi...@gmail.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Comment-Date: Mon, 09 Dec 2019 16:28:15 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9124: ImpalaServer and ClientRequestState refactoring
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14755 )
Change subject: IMPALA-9124: ImpalaServer and ClientRequestState refactoring
......................................................................
Patch Set 2:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/5271/ DRY_RUN=true
--
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 <st...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Comment-Date: Thu, 21 Nov 2019 20:05:32 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9124: ImpalaServer and ClientRequestState refactoring
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14755 )
Change subject: IMPALA-9124: ImpalaServer and ClientRequestState refactoring
......................................................................
Patch Set 1:
Build Failed
https://jenkins.impala.io/job/gerrit-code-review-checks/5089/ : Initial code review checks failed. See linked job for details on the failure.
--
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: 1
Gerrit-Owner: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Wed, 20 Nov 2019 21:41:10 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9124: ImpalaServer and ClientRequestState refactoring
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14755 )
Change subject: IMPALA-9124: ImpalaServer and ClientRequestState refactoring
......................................................................
Patch Set 9:
Build Failed
https://jenkins.impala.io/job/gerrit-code-review-checks/5193/ : Initial code review checks failed. See linked job for details on the failure.
--
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: 9
Gerrit-Owner: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Comment-Date: Wed, 04 Dec 2019 04:07:35 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9124: ImpalaServer and ClientRequestState refactoring
Posted by "Sahil Takiar (Code Review)" <ge...@cloudera.org>.
Hello Lars Volker, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/14755
to look at the new patch set (#2).
Change subject: IMPALA-9124: ImpalaServer and ClientRequestState refactoring
......................................................................
IMPALA-9124: ImpalaServer and ClientRequestState refactoring
Re-factoring several areas of the ImpalaServer and ClientRequestState
in preparation for future work required for transparent query retries.
Re-factored TExecRequest ownership in ClientRequestState. Previously,
the TExecRequest was passed to the ClientRequestState via the
Exec(TExecRequest) method. Now it is passed via the constructor and its
lifetime is managed by an unique_ptr. Using an unique_ptr avoids a copy of
the TExecRequest that was happening in
ClientRequestState::Exec(TExecRequest) and clarifies the ownership /
lifecycle of a TExecRequest.
Re-factored ImpalaServer::UnregisterQuery into multiple methods. Made
ClientRequestStateMap a dedicated class that extends ShardedQueryMap
with additional methods to add and delete a ClientRequestState. The
re-factoring breaks up the large UnregisterQuery method into multiple
smaller method and adds some additional code documentation.
Re-factored ImpalaHttpHandler's usage of the ClientRequestStateMap so
that it tracks its own list of running query ids / ClientRequestStates
rather than using the one owned by ImpalaServer. This is particularly
important for transparent query retries because retrying a query causes
duplicate ClientRequestStates to be added to the
ImpalaServer::client_request_state_map_.
Testing:
* Ran core tests
Change-Id: Ib92c932a69802225af3fd9bf15f85c85317acaca
---
M be/src/service/child-query.cc
A be/src/service/client-request-state-map.h
M be/src/service/client-request-state.cc
M be/src/service/client-request-state.h
M be/src/service/impala-http-handler.cc
M be/src/service/impala-http-handler.h
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
8 files changed, 230 insertions(+), 118 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/55/14755/2
--
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: newpatchset
Gerrit-Change-Id: Ib92c932a69802225af3fd9bf15f85c85317acaca
Gerrit-Change-Number: 14755
Gerrit-PatchSet: 2
Gerrit-Owner: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
[Impala-ASF-CR] IMPALA-9124: ImpalaServer and ClientRequestState refactoring
Posted by "Sahil Takiar (Code Review)" <ge...@cloudera.org>.
Hello Michael Ho, Lars Volker, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/14755
to look at the new patch set (#8).
Change subject: IMPALA-9124: ImpalaServer and ClientRequestState refactoring
......................................................................
IMPALA-9124: ImpalaServer and ClientRequestState refactoring
Re-factoring several areas of the ImpalaServer and ClientRequestState
in preparation for future work required for transparent query retries.
Re-factored TExecRequest ownership in ClientRequestState. Previously,
the TExecRequest was passed to the ClientRequestState via the
Exec(TExecRequest) method. Now it is created and owned by the
ClientRequestState and only exposed as a constant reference. It is
stored as a plain member variable.
Re-factored ImpalaServer::UnregisterQuery into multiple methods. Made
ClientRequestStateMap a dedicated class that extends ShardedQueryMap
with additional methods to add and delete a ClientRequestState. The
re-factoring breaks up the large UnregisterQuery method into multiple
smaller method and adds some additional code documentation.
Re-factored ImpalaServer::client_request_state_map_ into a separate
class.
Testing:
* Ran core tests
Change-Id: Ib92c932a69802225af3fd9bf15f85c85317acaca
---
A be/src/service/client-request-state-map.h
M be/src/service/client-request-state.cc
M be/src/service/client-request-state.h
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
5 files changed, 150 insertions(+), 60 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/55/14755/8
--
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: newpatchset
Gerrit-Change-Id: Ib92c932a69802225af3fd9bf15f85c85317acaca
Gerrit-Change-Number: 14755
Gerrit-PatchSet: 8
Gerrit-Owner: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
[Impala-ASF-CR] IMPALA-9124: ImpalaServer and ClientRequestState refactoring
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14755 )
Change subject: IMPALA-9124: ImpalaServer and ClientRequestState refactoring
......................................................................
Patch Set 8:
Build Failed
https://jenkins.impala.io/job/gerrit-code-review-checks/5192/ : Initial code review checks failed. See linked job for details on the failure.
--
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: 8
Gerrit-Owner: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Comment-Date: Wed, 04 Dec 2019 04:06:29 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9124: ImpalaServer and ClientRequestState refactoring
Posted by "Sahil Takiar (Code Review)" <ge...@cloudera.org>.
Sahil Takiar has posted comments on this change. ( http://gerrit.cloudera.org:8080/14755 )
Change subject: IMPALA-9124: ImpalaServer and ClientRequestState refactoring
......................................................................
Patch Set 7:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/14755/7/be/src/service/client-request-state.h
File be/src/service/client-request-state.h:
http://gerrit.cloudera.org:8080/#/c/14755/7/be/src/service/client-request-state.h@231
PS7, Line 231: /// Contents are only valid after InitExecRequest(TQueryCtx) initializes the TExecRequest.
> line too long (92 > 90)
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: 7
Gerrit-Owner: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Comment-Date: Wed, 04 Dec 2019 03:44:51 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-9124: ImpalaServer and ClientRequestState refactoring
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14755 )
Change subject: IMPALA-9124: ImpalaServer and ClientRequestState refactoring
......................................................................
Patch Set 5:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/5166/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
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: 5
Gerrit-Owner: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Comment-Date: Wed, 27 Nov 2019 23:40:03 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9124: ImpalaServer and ClientRequestState refactoring
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14755 )
Change subject: IMPALA-9124: ImpalaServer and ClientRequestState refactoring
......................................................................
Patch Set 4: Verified+1
--
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: 4
Gerrit-Owner: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Comment-Date: Tue, 26 Nov 2019 06:17:33 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9124: ImpalaServer and ClientRequestState refactoring
Posted by "Sahil Takiar (Code Review)" <ge...@cloudera.org>.
Sahil Takiar has posted comments on this change. ( http://gerrit.cloudera.org:8080/14755 )
Change subject: IMPALA-9124: ImpalaServer and ClientRequestState refactoring
......................................................................
Patch Set 5:
I decided to remove the changes to the ImpalaHttpHandler. The changes don't make much sense without the necessary transparent query retry code + I found a few issues in the approach while working on my query retry POC.
--
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: 5
Gerrit-Owner: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Comment-Date: Wed, 27 Nov 2019 22:56:19 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9124: ImpalaServer and ClientRequestState refactoring
Posted by "Sahil Takiar (Code Review)" <ge...@cloudera.org>.
Sahil Takiar has posted comments on this change. ( http://gerrit.cloudera.org:8080/14755 )
Change subject: IMPALA-9124: ImpalaServer and ClientRequestState refactoring
......................................................................
Patch Set 5:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/14755/5/be/src/service/client-request-state.h
File be/src/service/client-request-state.h:
http://gerrit.cloudera.org:8080/#/c/14755/5/be/src/service/client-request-state.h@76
PS5, Line 76: exec_request
> nit: update comment
Done
http://gerrit.cloudera.org:8080/#/c/14755/5/be/src/service/client-request-state.h@192
PS5, Line 192: Load
> I think "Load" is slightly misleading here, it suggests that this comes fro
Done
http://gerrit.cloudera.org:8080/#/c/14755/5/be/src/service/client-request-state.h@231
PS5, Line 231: const TExecRequest& exec_request() const {
> This could DCHECK that the exec_request_ has been initialized. Then you cou
I'm actually about to change this again in an upcoming patch, so I'm not sure it is worth it. Would rather do this in a follow up patch.
http://gerrit.cloudera.org:8080/#/c/14755/5/be/src/service/client-request-state.h@471
PS5, Line 471: /// is loaded in LoadExecRequest(TQueryCtx).
> Mention that it must not be used before initializing it?
Done
http://gerrit.cloudera.org:8080/#/c/14755/5/be/src/service/impala-server.h
File be/src/service/impala-server.h:
http://gerrit.cloudera.org:8080/#/c/14755/5/be/src/service/impala-server.h@1091
PS5, Line 1091: shared_ptr
> This is now confusing.
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: 5
Gerrit-Owner: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Comment-Date: Wed, 04 Dec 2019 03:42:53 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-9124: ImpalaServer and ClientRequestState refactoring
Posted by "Sahil Takiar (Code Review)" <ge...@cloudera.org>.
Hello Michael Ho, Lars Volker, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/14755
to look at the new patch set (#11).
Change subject: IMPALA-9124: ImpalaServer and ClientRequestState refactoring
......................................................................
IMPALA-9124: ImpalaServer and ClientRequestState refactoring
Re-factoring several areas of the ImpalaServer and ClientRequestState
in preparation for future work required for transparent query retries.
Re-factored TExecRequest ownership in ClientRequestState. Previously,
the TExecRequest was passed to the ClientRequestState via the
Exec(TExecRequest) method. Now it is created and owned by the
ClientRequestState and only exposed as a constant reference. It is
stored as a plain member variable.
Re-factored ImpalaServer::UnregisterQuery into multiple methods. Made
ClientRequestStateMap a dedicated class that extends ShardedQueryMap
with additional methods to add and delete a ClientRequestState. The
re-factoring breaks up the large UnregisterQuery method into multiple
smaller method and adds some additional code documentation.
Re-factored ImpalaServer::client_request_state_map_ into a separate
class.
Testing:
* Ran core tests
Change-Id: Ib92c932a69802225af3fd9bf15f85c85317acaca
---
A be/src/service/client-request-state-map.h
M be/src/service/client-request-state.cc
M be/src/service/client-request-state.h
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
5 files changed, 150 insertions(+), 60 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/55/14755/11
--
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: newpatchset
Gerrit-Change-Id: Ib92c932a69802225af3fd9bf15f85c85317acaca
Gerrit-Change-Number: 14755
Gerrit-PatchSet: 11
Gerrit-Owner: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <mi...@gmail.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
[Impala-ASF-CR] IMPALA-9124: ImpalaServer and ClientRequestState refactoring
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14755 )
Change subject: IMPALA-9124: ImpalaServer and ClientRequestState refactoring
......................................................................
Patch Set 7:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/14755/7/be/src/service/client-request-state.h
File be/src/service/client-request-state.h:
http://gerrit.cloudera.org:8080/#/c/14755/7/be/src/service/client-request-state.h@231
PS7, Line 231: /// Contents are only valid after InitExecRequest(TQueryCtx) initializes the TExecRequest.
line too long (92 > 90)
--
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: 7
Gerrit-Owner: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Comment-Date: Wed, 04 Dec 2019 03:43:19 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-9124: ImpalaServer and ClientRequestState refactoring
Posted by "Sahil Takiar (Code Review)" <ge...@cloudera.org>.
Hello Michael Ho, Lars Volker, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/14755
to look at the new patch set (#5).
Change subject: IMPALA-9124: ImpalaServer and ClientRequestState refactoring
......................................................................
IMPALA-9124: ImpalaServer and ClientRequestState refactoring
Re-factoring several areas of the ImpalaServer and ClientRequestState
in preparation for future work required for transparent query retries.
Re-factored TExecRequest ownership in ClientRequestState. Previously,
the TExecRequest was passed to the ClientRequestState via the
Exec(TExecRequest) method. Now it is created and owned by the
ClientRequestState and only exposed as a constant reference. It is
stored as a plain member variable.
Re-factored ImpalaServer::UnregisterQuery into multiple methods. Made
ClientRequestStateMap a dedicated class that extends ShardedQueryMap
with additional methods to add and delete a ClientRequestState. The
re-factoring breaks up the large UnregisterQuery method into multiple
smaller method and adds some additional code documentation.
Re-factored ImpalaServer::client_request_state_map_ into a separate
class.
Testing:
* Ran core tests
Change-Id: Ib92c932a69802225af3fd9bf15f85c85317acaca
---
A be/src/service/client-request-state-map.h
M be/src/service/client-request-state.cc
M be/src/service/client-request-state.h
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
5 files changed, 143 insertions(+), 57 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/55/14755/5
--
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: newpatchset
Gerrit-Change-Id: Ib92c932a69802225af3fd9bf15f85c85317acaca
Gerrit-Change-Number: 14755
Gerrit-PatchSet: 5
Gerrit-Owner: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
[Impala-ASF-CR] IMPALA-9124: ImpalaServer and ClientRequestState refactoring
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14755 )
Change subject: IMPALA-9124: ImpalaServer and ClientRequestState refactoring
......................................................................
Patch Set 10: Code-Review+2
--
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: 10
Gerrit-Owner: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <mi...@gmail.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Comment-Date: Mon, 09 Dec 2019 15:30:47 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9124: ImpalaServer and ClientRequestState refactoring
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14755 )
Change subject: IMPALA-9124: ImpalaServer and ClientRequestState refactoring
......................................................................
Patch Set 2: Verified-1
Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/5271/
--
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 <st...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Comment-Date: Fri, 22 Nov 2019 06:05:35 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9124: ImpalaServer and ClientRequestState refactoring
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/14755 )
Change subject: IMPALA-9124: ImpalaServer and ClientRequestState refactoring
......................................................................
IMPALA-9124: ImpalaServer and ClientRequestState refactoring
Re-factoring several areas of the ImpalaServer and ClientRequestState
in preparation for future work required for transparent query retries.
Re-factored TExecRequest ownership in ClientRequestState. Previously,
the TExecRequest was passed to the ClientRequestState via the
Exec(TExecRequest) method. Now it is created and owned by the
ClientRequestState and only exposed as a constant reference. It is
stored as a plain member variable.
Re-factored ImpalaServer::UnregisterQuery into multiple methods. Made
ClientRequestStateMap a dedicated class that extends ShardedQueryMap
with additional methods to add and delete a ClientRequestState. The
re-factoring breaks up the large UnregisterQuery method into multiple
smaller method and adds some additional code documentation.
Re-factored ImpalaServer::client_request_state_map_ into a separate
class.
Testing:
* Ran core tests
Change-Id: Ib92c932a69802225af3fd9bf15f85c85317acaca
Reviewed-on: http://gerrit.cloudera.org:8080/14755
Reviewed-by: Sahil Takiar <st...@cloudera.com>
Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
A be/src/service/client-request-state-map.h
M be/src/service/client-request-state.cc
M be/src/service/client-request-state.h
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
5 files changed, 150 insertions(+), 60 deletions(-)
Approvals:
Sahil Takiar: Looks good to me, approved
Impala Public Jenkins: Verified
--
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: merged
Gerrit-Change-Id: Ib92c932a69802225af3fd9bf15f85c85317acaca
Gerrit-Change-Number: 14755
Gerrit-PatchSet: 12
Gerrit-Owner: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <mi...@gmail.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
[Impala-ASF-CR] IMPALA-9124: ImpalaServer and ClientRequestState refactoring
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14755 )
Change subject: IMPALA-9124: ImpalaServer and ClientRequestState refactoring
......................................................................
Patch Set 11:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/5230/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
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: 11
Gerrit-Owner: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <mi...@gmail.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Comment-Date: Mon, 09 Dec 2019 16:56:16 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9124: ImpalaServer and ClientRequestState refactoring
Posted by "Sahil Takiar (Code Review)" <ge...@cloudera.org>.
Hello Michael Ho, Lars Volker, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/14755
to look at the new patch set (#4).
Change subject: IMPALA-9124: ImpalaServer and ClientRequestState refactoring
......................................................................
IMPALA-9124: ImpalaServer and ClientRequestState refactoring
Re-factoring several areas of the ImpalaServer and ClientRequestState
in preparation for future work required for transparent query retries.
Re-factored TExecRequest ownership in ClientRequestState. Previously,
the TExecRequest was passed to the ClientRequestState via the
Exec(TExecRequest) method. Now it is created and owned by the
ClientRequestState and only exposed as a constant reference. It is
stored as a plain member variable.
Re-factored ImpalaServer::UnregisterQuery into multiple methods. Made
ClientRequestStateMap a dedicated class that extends ShardedQueryMap
with additional methods to add and delete a ClientRequestState. The
re-factoring breaks up the large UnregisterQuery method into multiple
smaller method and adds some additional code documentation.
Re-factored ImpalaHttpHandler's usage of the ClientRequestStateMap so
that it tracks its own list of running query ids / ClientRequestStates
rather than using the one owned by ImpalaServer. This is particularly
important for transparent query retries because retrying a query causes
duplicate ClientRequestStates to be added to the
ImpalaServer::client_request_state_map_.
Testing:
* Ran core tests
Change-Id: Ib92c932a69802225af3fd9bf15f85c85317acaca
---
A be/src/service/client-request-state-map.h
M be/src/service/client-request-state.cc
M be/src/service/client-request-state.h
M be/src/service/impala-http-handler.cc
M be/src/service/impala-http-handler.h
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
7 files changed, 179 insertions(+), 61 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/55/14755/4
--
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: newpatchset
Gerrit-Change-Id: Ib92c932a69802225af3fd9bf15f85c85317acaca
Gerrit-Change-Number: 14755
Gerrit-PatchSet: 4
Gerrit-Owner: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
[Impala-ASF-CR] IMPALA-9124: ImpalaServer and ClientRequestState refactoring
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14755 )
Change subject: IMPALA-9124: ImpalaServer and ClientRequestState refactoring
......................................................................
Patch Set 7:
Build Failed
https://jenkins.impala.io/job/gerrit-code-review-checks/5191/ : Initial code review checks failed. See linked job for details on the failure.
--
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: 7
Gerrit-Owner: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Comment-Date: Wed, 04 Dec 2019 04:03:47 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9124: ImpalaServer and ClientRequestState refactoring
Posted by "Sahil Takiar (Code Review)" <ge...@cloudera.org>.
Sahil Takiar has uploaded a new patch set (#6). ( http://gerrit.cloudera.org:8080/14755 )
Change subject: IMPALA-9124: ImpalaServer and ClientRequestState refactoring
......................................................................
IMPALA-9124: ImpalaServer and ClientRequestState refactoring
Re-factoring several areas of the ImpalaServer and ClientRequestState
in preparation for future work required for transparent query retries.
Re-factored TExecRequest ownership in ClientRequestState. Previously,
the TExecRequest was passed to the ClientRequestState via the
Exec(TExecRequest) method. Now it is created and owned by the
ClientRequestState and only exposed as a constant reference. It is
stored as a plain member variable.
Re-factored ImpalaServer::UnregisterQuery into multiple methods. Made
ClientRequestStateMap a dedicated class that extends ShardedQueryMap
with additional methods to add and delete a ClientRequestState. The
re-factoring breaks up the large UnregisterQuery method into multiple
smaller method and adds some additional code documentation.
Re-factored ImpalaServer::client_request_state_map_ into a separate
class.
Testing:
* Ran core tests
Change-Id: Ib92c932a69802225af3fd9bf15f85c85317acaca
---
A be/src/service/client-request-state-map.h
M be/src/service/client-request-state.cc
M be/src/service/client-request-state.h
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
5 files changed, 143 insertions(+), 57 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/55/14755/6
--
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: newpatchset
Gerrit-Change-Id: Ib92c932a69802225af3fd9bf15f85c85317acaca
Gerrit-Change-Number: 14755
Gerrit-PatchSet: 6
Gerrit-Owner: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
[Impala-ASF-CR] IMPALA-9124: ImpalaServer and ClientRequestState refactoring
Posted by "Sahil Takiar (Code Review)" <ge...@cloudera.org>.
Sahil Takiar has posted comments on this change. ( http://gerrit.cloudera.org:8080/14755 )
Change subject: IMPALA-9124: ImpalaServer and ClientRequestState refactoring
......................................................................
Patch Set 1:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/14755/1/be/src/service/client-request-state-map.h
File be/src/service/client-request-state-map.h:
http://gerrit.cloudera.org:8080/#/c/14755/1/be/src/service/client-request-state-map.h@35
PS1, Line 35: const std::shared_ptr<ClientRequestState>& request_state) {
> We should take the shared_ptr by value since we are going to retain a copy
Done
http://gerrit.cloudera.org:8080/#/c/14755/1/be/src/service/client-request-state-map.h@44
PS1, Line 44: ss << "query id " << PrintId(query_id) << " already exists";
> This could use "Substitute()" to be a bit more concise
Done
http://gerrit.cloudera.org:8080/#/c/14755/1/be/src/service/client-request-state-map.h@55
PS1, Line 55: Status DeleteClientRequestState(
> You could add an overload without the request_state argument to make the us
Done
http://gerrit.cloudera.org:8080/#/c/14755/1/be/src/service/impala-http-handler.h
File be/src/service/impala-http-handler.h:
http://gerrit.cloudera.org:8080/#/c/14755/1/be/src/service/impala-http-handler.h@55
PS1, Line 55: /// Tracks the ClientRequestStates of the currently registered queries.
> Can you add to the comment why we store them here as well as in the ImpalaS
Done
http://gerrit.cloudera.org:8080/#/c/14755/1/be/src/service/impala-server.h
File be/src/service/impala-server.h:
http://gerrit.cloudera.org:8080/#/c/14755/1/be/src/service/impala-server.h@1084
PS1, Line 1084: ClientRequestState is owned
> What does this mean in the context of shared_ptr? If it is owned, shouldn't
Changed to unique_ptr and did a bit more re-factoring so that the ClientRequestState creates the TExecRequest internally.
--
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: 1
Gerrit-Owner: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Comment-Date: Thu, 21 Nov 2019 20:02:38 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-9124: ImpalaServer and ClientRequestState refactoring
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14755 )
Change subject: IMPALA-9124: ImpalaServer and ClientRequestState refactoring
......................................................................
Patch Set 8:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/14755/8/be/src/service/client-request-state.h
File be/src/service/client-request-state.h:
http://gerrit.cloudera.org:8080/#/c/14755/8/be/src/service/client-request-state.h@231
PS8, Line 231: /// Contents are only valid after InitExecRequest(TQueryCtx) initializes the
line has trailing whitespace
--
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: 8
Gerrit-Owner: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Comment-Date: Wed, 04 Dec 2019 03:45:22 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-9124: ImpalaServer and ClientRequestState refactoring
Posted by "Lars Volker (Code Review)" <ge...@cloudera.org>.
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 <st...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Comment-Date: Thu, 21 Nov 2019 22:33:57 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-9124: ImpalaServer and ClientRequestState refactoring
Posted by "Sahil Takiar (Code Review)" <ge...@cloudera.org>.
Hello Michael Ho, Lars Volker, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/14755
to look at the new patch set (#3).
Change subject: IMPALA-9124: ImpalaServer and ClientRequestState refactoring
......................................................................
IMPALA-9124: ImpalaServer and ClientRequestState refactoring
Re-factoring several areas of the ImpalaServer and ClientRequestState
in preparation for future work required for transparent query retries.
Re-factored TExecRequest ownership in ClientRequestState. Previously,
the TExecRequest was passed to the ClientRequestState via the
Exec(TExecRequest) method. Now it is passed via the constructor and its
lifetime is managed by an unique_ptr. Using an unique_ptr avoids a copy of
the TExecRequest that was happening in
ClientRequestState::Exec(TExecRequest) and clarifies the ownership /
lifecycle of a TExecRequest.
Re-factored ImpalaServer::UnregisterQuery into multiple methods. Made
ClientRequestStateMap a dedicated class that extends ShardedQueryMap
with additional methods to add and delete a ClientRequestState. The
re-factoring breaks up the large UnregisterQuery method into multiple
smaller method and adds some additional code documentation.
Re-factored ImpalaHttpHandler's usage of the ClientRequestStateMap so
that it tracks its own list of running query ids / ClientRequestStates
rather than using the one owned by ImpalaServer. This is particularly
important for transparent query retries because retrying a query causes
duplicate ClientRequestStates to be added to the
ImpalaServer::client_request_state_map_.
Testing:
* Ran core tests
Change-Id: Ib92c932a69802225af3fd9bf15f85c85317acaca
---
A be/src/service/client-request-state-map.h
M be/src/service/client-request-state.cc
M be/src/service/client-request-state.h
M be/src/service/impala-http-handler.cc
M be/src/service/impala-http-handler.h
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
7 files changed, 179 insertions(+), 61 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/55/14755/3
--
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: newpatchset
Gerrit-Change-Id: Ib92c932a69802225af3fd9bf15f85c85317acaca
Gerrit-Change-Number: 14755
Gerrit-PatchSet: 3
Gerrit-Owner: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
[Impala-ASF-CR] IMPALA-9124: ImpalaServer and ClientRequestState refactoring
Posted by "Lars Volker (Code Review)" <ge...@cloudera.org>.
Lars Volker has posted comments on this change. ( http://gerrit.cloudera.org:8080/14755 )
Change subject: IMPALA-9124: ImpalaServer and ClientRequestState refactoring
......................................................................
Patch Set 5:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/14755/5/be/src/service/client-request-state.h
File be/src/service/client-request-state.h:
http://gerrit.cloudera.org:8080/#/c/14755/5/be/src/service/client-request-state.h@76
PS5, Line 76: exec_request
nit: update comment
http://gerrit.cloudera.org:8080/#/c/14755/5/be/src/service/client-request-state.h@192
PS5, Line 192: Load
I think "Load" is slightly misleading here, it suggests that this comes from some kind of store? How about InitExecRequest()?
http://gerrit.cloudera.org:8080/#/c/14755/5/be/src/service/client-request-state.h@231
PS5, Line 231: const TExecRequest& exec_request() const {
This could DCHECK that the exec_request_ has been initialized. Then you could revert to calling it in the .cc file.
http://gerrit.cloudera.org:8080/#/c/14755/5/be/src/service/client-request-state.h@471
PS5, Line 471: /// is loaded in LoadExecRequest(TQueryCtx).
Mention that it must not be used before initializing it?
http://gerrit.cloudera.org:8080/#/c/14755/5/be/src/service/impala-server.h
File be/src/service/impala-server.h:
http://gerrit.cloudera.org:8080/#/c/14755/5/be/src/service/impala-server.h@1091
PS5, Line 1091: shared_ptr
This is now confusing.
--
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: 5
Gerrit-Owner: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Comment-Date: Wed, 04 Dec 2019 00:35:12 +0000
Gerrit-HasComments: Yes