You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Sailesh Mukil (Code Review)" <ge...@cloudera.org> on 2017/09/18 19:11:48 UTC
[Impala-ASF-CR] IMPALA-4671: Replace kudu::ServicePool with one that uses Impala threads
Sailesh Mukil has uploaded a new change for review.
http://gerrit.cloudera.org:8080/8094
Change subject: IMPALA-4671: Replace kudu::ServicePool with one that uses Impala threads
......................................................................
IMPALA-4671: Replace kudu::ServicePool with one that uses Impala threads
Kudu's ServicePool uses Kudu's Thread class to service RPC requests.
While this works well, the threads it creates aren't monitored by
Impala's ThreadMgr subsystem.
Eventually we'd like to standardise on one thread class between
projects, but for now it would be useful to reimplement ServicePool
in terms of our thread class.
The reactor threads and acceptor threads will still be Kudu threads,
but those are less likely to do substantial work, so having them
missing from monitoring isn't a big problem.
This patch just changes kudu::Thread to impala::Thread and changes
the container pointer type from scoped_refptr to unique_ptr.
Testing: Ported it on top of https://gerrit.cloudera.org/#/c/8023/
and verified that the Service pool works as expected.
Change-Id: Ibb75c91d4c754f136bcb4b51dd66e2d933b14b35
---
M be/src/kudu/rpc/service_pool.cc
M be/src/kudu/rpc/service_pool.h
2 files changed, 12 insertions(+), 7 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/94/8094/1
--
To view, visit http://gerrit.cloudera.org:8080/8094
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ibb75c91d4c754f136bcb4b51dd66e2d933b14b35
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
[Impala-ASF-CR] IMPALA-4671: Replace kudu::ServicePool with one that uses Impala threads
Posted by "Henry Robinson (Code Review)" <ge...@cloudera.org>.
Henry Robinson has posted comments on this change.
Change subject: IMPALA-4671: Replace kudu::ServicePool with one that uses Impala threads
......................................................................
Patch Set 1:
Why not make a separate implementation of a service pool, rather than modify Kudu's? See https://github.com/henryr/Impala/commit/1e1810dad63b821d3d530d09debf47e3a8f4a570 for an example of how that could work. Then you have a much better chance of integrating the service pool with Impala's observability subsystems (again, there are examples in that commit).
--
To view, visit http://gerrit.cloudera.org:8080/8094
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibb75c91d4c754f136bcb4b51dd66e2d933b14b35
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4671: Replace kudu::ServicePool with one that uses Impala threads
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Sailesh Mukil has posted comments on this change.
Change subject: IMPALA-4671: Replace kudu::ServicePool with one that uses Impala threads
......................................................................
Patch Set 1:
> Why not make a separate implementation of a service pool, rather
> than modify Kudu's? See https://github.com/henryr/Impala/commit/1e1810dad63b821d3d530d09debf47e3a8f4a570
> for an example of how that could work. Then you have a much better
> chance of integrating the service pool with Impala's observability
> subsystems (again, there are examples in that commit).
Thanks for the suggestion and the code pointer, Henry!
The way I see it, there are some pros and cons to having a parallel ServicePool implementation.
Pros:
* Better observability. (Huge win)
* We avoid modifying more Kudu code.
Cons:
* We need to periodically check against kudu::ServicePool if there were any bugs and re-implement the fix on our side. (Not too bad, but still a task)
* This might be obsolete after we standardize on a utils library between Impala and Kudu. (But this may take a while to happen)
* Requires more time to implement and review.
Given these, I'm still leaning towards your suggestion of having an Impala native ServicePool. Let me go through your patch before I make a call.
In the mean time, other reviewers should feel free to weigh in.
--
To view, visit http://gerrit.cloudera.org:8080/8094
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibb75c91d4c754f136bcb4b51dd66e2d933b14b35
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4671: Replace kudu::ServicePool with one that uses Impala threads
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Sailesh Mukil has abandoned this change. ( http://gerrit.cloudera.org:8080/8094 )
Change subject: IMPALA-4671: Replace kudu::ServicePool with one that uses Impala threads
......................................................................
Abandoned
Will update with a patch that uses an Impala native ServicePool.
--
To view, visit http://gerrit.cloudera.org:8080/8094
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: abandon
Gerrit-Change-Id: Ibb75c91d4c754f136bcb4b51dd66e2d933b14b35
Gerrit-Change-Number: 8094
Gerrit-PatchSet: 1
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>