[Impala-ASF-CR] IMPALA-4671: (part-2) Replace kudu::ServicePool with one that uses Impala threads

2017-12-07 Thread Impala Public Jenkins (Code Review) 
Impala Public Jenkins has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8472 )

Change subject: IMPALA-4671: (part-2) Replace kudu::ServicePool with one that 
uses Impala threads
..


Patch Set 9: Verified+1


--
To view, visit http://gerrit.cloudera.org:8080/8472
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia3e0fa9ae22c4803b0cea736eb35e333e2671448
Gerrit-Change-Number: 8472
Gerrit-PatchSet: 9
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Comment-Date: Thu, 07 Dec 2017 18:29:58 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4671: (part-2) Replace kudu::ServicePool with one that uses Impala threads

2017-12-07 Thread Impala Public Jenkins (Code Review) 
Impala Public Jenkins has submitted this change and it was merged. ( 
http://gerrit.cloudera.org:8080/8472 )

Change subject: IMPALA-4671: (part-2) Replace kudu::ServicePool with one that 
uses Impala threads
..

IMPALA-4671: (part-2) Replace kudu::ServicePool with one that uses Impala 
threads

The KuduRPC subsystem uses kudu::ServicePool to service all incoming
RPCs. Since this lives inside the Kudu codebase, all instrumentation
of RPCs flowing through the KuduRPC subsystem is not visible to Impala,
as Kudu does not export this instrumentation, but only maintains it
internally within kudu::ServicePool.

In order to reliably view the instrumentation of all RPCs flowing through
KRPC, one option is to modify kudu::ServicePool to take their
instrumentation and display it on our webpages, which is very invasive.

A second option is to have a parallel implementation of kudu::ServicePool
which for all purposes behaves the same way, but instead has this extra
code that displays this instrumentation in our webpages.

This patch is Part 2 of the second option.

In Part 1, we simply copied the code from kudu::ServicePool into the Impala
namespace. In this patch, we rename the redundant kudu::ServicePool
(that was copied into be/src/rpc/) to ImpalaServicePool. We also expose
the instrumentaion of the ImpalaServicePool as JSON.

Now, the threads in use by the service pool will also be visible in
the /threadz page.

This patch however does not display instrumentation on the Webpages yet.
In a future patch, we will add that through the ImpalaServicePool and the
RpcMgr. Tracked by IMPALA-6269

This patch is partially based on an abandoned patch by Henry Robinson.

Change-Id: Ia3e0fa9ae22c4803b0cea736eb35e333e2671448
Reviewed-on: http://gerrit.cloudera.org:8080/8472
Reviewed-by: Sailesh Mukil 
Tested-by: Impala Public Jenkins
---
M be/src/rpc/CMakeLists.txt
M be/src/rpc/impala-service-pool.cc
M be/src/rpc/impala-service-pool.h
M be/src/rpc/rpc-mgr.cc
M be/src/rpc/rpc-mgr.h
M be/src/rpc/rpc-trace.h
6 files changed, 119 insertions(+), 141 deletions(-)

Approvals:
  Sailesh Mukil: Looks good to me, approved
  Impala Public Jenkins: Verified

--
To view, visit http://gerrit.cloudera.org:8080/8472
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Ia3e0fa9ae22c4803b0cea736eb35e333e2671448
Gerrit-Change-Number: 8472
Gerrit-PatchSet: 10
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil

[Impala-ASF-CR] IMPALA-4671: (part-2) Replace kudu::ServicePool with one that uses Impala threads

2017-12-07 Thread Impala Public Jenkins (Code Review) 
Impala Public Jenkins has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8472 )

Change subject: IMPALA-4671: (part-2) Replace kudu::ServicePool with one that 
uses Impala threads
..


Patch Set 9:

Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/1587/


--
To view, visit http://gerrit.cloudera.org:8080/8472
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia3e0fa9ae22c4803b0cea736eb35e333e2671448
Gerrit-Change-Number: 8472
Gerrit-PatchSet: 9
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Comment-Date: Thu, 07 Dec 2017 14:51:16 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4671: (part-2) Replace kudu::ServicePool with one that uses Impala threads

2017-12-07 Thread Sailesh Mukil (Code Review) 
Hello Michael Ho, Lars Volker, Dan Hecht, Impala Public Jenkins,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/8472

to look at the new patch set (#9).

Change subject: IMPALA-4671: (part-2) Replace kudu::ServicePool with one that 
uses Impala threads
..

IMPALA-4671: (part-2) Replace kudu::ServicePool with one that uses Impala 
threads

The KuduRPC subsystem uses kudu::ServicePool to service all incoming
RPCs. Since this lives inside the Kudu codebase, all instrumentation
of RPCs flowing through the KuduRPC subsystem is not visible to Impala,
as Kudu does not export this instrumentation, but only maintains it
internally within kudu::ServicePool.

In order to reliably view the instrumentation of all RPCs flowing through
KRPC, one option is to modify kudu::ServicePool to take their
instrumentation and display it on our webpages, which is very invasive.

A second option is to have a parallel implementation of kudu::ServicePool
which for all purposes behaves the same way, but instead has this extra
code that displays this instrumentation in our webpages.

This patch is Part 2 of the second option.

In Part 1, we simply copied the code from kudu::ServicePool into the Impala
namespace. In this patch, we rename the redundant kudu::ServicePool
(that was copied into be/src/rpc/) to ImpalaServicePool. We also expose
the instrumentaion of the ImpalaServicePool as JSON.

Now, the threads in use by the service pool will also be visible in
the /threadz page.

This patch however does not display instrumentation on the Webpages yet.
In a future patch, we will add that through the ImpalaServicePool and the
RpcMgr. Tracked by IMPALA-6269

This patch is partially based on an abandoned patch by Henry Robinson.

Change-Id: Ia3e0fa9ae22c4803b0cea736eb35e333e2671448
---
M be/src/rpc/CMakeLists.txt
M be/src/rpc/impala-service-pool.cc
M be/src/rpc/impala-service-pool.h
M be/src/rpc/rpc-mgr.cc
M be/src/rpc/rpc-mgr.h
M be/src/rpc/rpc-trace.h
6 files changed, 119 insertions(+), 141 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/72/8472/9
--
To view, visit http://gerrit.cloudera.org:8080/8472
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ia3e0fa9ae22c4803b0cea736eb35e333e2671448
Gerrit-Change-Number: 8472
Gerrit-PatchSet: 9
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil

[Impala-ASF-CR] IMPALA-4671: (part-2) Replace kudu::ServicePool with one that uses Impala threads

2017-12-05 Thread Sailesh Mukil (Code Review) 
Sailesh Mukil has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8472 )

Change subject: IMPALA-4671: (part-2) Replace kudu::ServicePool with one that 
uses Impala threads
..


Patch Set 7: Code-Review+2

(1 comment)

Carry +2.

http://gerrit.cloudera.org:8080/#/c/8472/7/be/src/rpc/impala-service-pool.cc
File be/src/rpc/impala-service-pool.cc:

http://gerrit.cloudera.org:8080/#/c/8472/7/be/src/rpc/impala-service-pool.cc@194
PS7, Line 194: // NOLINT(*)
GVO failed because clang tidy incorrectly wanted the TRACE_TO() macro to fill 
in optional arguments, but it's not necessary here.



--
To view, visit http://gerrit.cloudera.org:8080/8472
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia3e0fa9ae22c4803b0cea736eb35e333e2671448
Gerrit-Change-Number: 8472
Gerrit-PatchSet: 7
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Comment-Date: Tue, 05 Dec 2017 20:36:53 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4671: (part-2) Replace kudu::ServicePool with one that uses Impala threads

2017-12-04 Thread Impala Public Jenkins (Code Review) 
Impala Public Jenkins has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8472 )

Change subject: IMPALA-4671: (part-2) Replace kudu::ServicePool with one that 
uses Impala threads
..


Patch Set 6:

Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/1566/


--
To view, visit http://gerrit.cloudera.org:8080/8472
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia3e0fa9ae22c4803b0cea736eb35e333e2671448
Gerrit-Change-Number: 8472
Gerrit-PatchSet: 6
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Comment-Date: Tue, 05 Dec 2017 05:14:29 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4671: (part-2) Replace kudu::ServicePool with one that uses Impala threads

2017-12-04 Thread Sailesh Mukil (Code Review) 
Hello Michael Ho, Lars Volker, Dan Hecht,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/8472

to look at the new patch set (#6).

Change subject: IMPALA-4671: (part-2) Replace kudu::ServicePool with one that 
uses Impala threads
..

IMPALA-4671: (part-2) Replace kudu::ServicePool with one that uses Impala 
threads

The KuduRPC subsystem uses kudu::ServicePool to service all incoming
RPCs. Since this lives inside the Kudu codebase, all instrumentation
of RPCs flowing through the KuduRPC subsystem is not visible to Impala,
as Kudu does not export this instrumentation, but only maintains it
internally within kudu::ServicePool.

In order to reliably view the instrumentation of all RPCs flowing through
KRPC, one option is to modify kudu::ServicePool to take their
instrumentation and display it on our webpages, which is very invasive.

A second option is to have a parallel implementation of kudu::ServicePool
which for all purposes behaves the same way, but instead has this extra
code that displays this instrumentation in our webpages.

This patch is Part 2 of the second option.

In Part 1, we simply copied the code from kudu::ServicePool into the Impala
namespace. In this patch, we rename the redundant kudu::ServicePool
(that was copied into be/src/rpc/) to ImpalaServicePool. We also expose
the instrumentaion of the ImpalaServicePool as JSON.

Now, the threads in use by the service pool will also be visible in
the /threadz page.

This patch however does not display instrumentation on the Webpages yet.
In a future patch, we will add that through the ImpalaServicePool and the
RpcMgr. Tracked by IMPALA-6269

This patch is partially based on an abandoned patch by Henry Robinson.

Change-Id: Ia3e0fa9ae22c4803b0cea736eb35e333e2671448
---
M be/src/rpc/CMakeLists.txt
M be/src/rpc/impala-service-pool.cc
M be/src/rpc/impala-service-pool.h
M be/src/rpc/rpc-mgr.cc
M be/src/rpc/rpc-mgr.h
M be/src/rpc/rpc-trace.h
6 files changed, 116 insertions(+), 138 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/72/8472/6
--
To view, visit http://gerrit.cloudera.org:8080/8472
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ia3e0fa9ae22c4803b0cea736eb35e333e2671448
Gerrit-Change-Number: 8472
Gerrit-PatchSet: 6
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil

[Impala-ASF-CR] IMPALA-4671: (part-2) Replace kudu::ServicePool with one that uses Impala threads

2017-12-04 Thread Sailesh Mukil (Code Review) 
Sailesh Mukil has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8472 )

Change subject: IMPALA-4671: (part-2) Replace kudu::ServicePool with one that 
uses Impala threads
..


Patch Set 6: Code-Review+2

(1 comment)

Rebase, carry +2.

http://gerrit.cloudera.org:8080/#/c/8472/5/be/src/rpc/impala-service-pool.h
File be/src/rpc/impala-service-pool.h:

http://gerrit.cloudera.org:8080/#/c/8472/5/be/src/rpc/impala-service-pool.h@76
PS5, Line 76: against c
> what resources?  Let's just remove that word since we've been trying to be
Done



--
To view, visit http://gerrit.cloudera.org:8080/8472
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia3e0fa9ae22c4803b0cea736eb35e333e2671448
Gerrit-Change-Number: 8472
Gerrit-PatchSet: 6
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Comment-Date: Tue, 05 Dec 2017 05:14:08 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4671: (part-2) Replace kudu::ServicePool with one that uses Impala threads

2017-12-04 Thread Dan Hecht (Code Review) 
Dan Hecht has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8472 )

Change subject: IMPALA-4671: (part-2) Replace kudu::ServicePool with one that 
uses Impala threads
..


Patch Set 5: Code-Review+2

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8472/5/be/src/rpc/impala-service-pool.h
File be/src/rpc/impala-service-pool.h:

http://gerrit.cloudera.org:8080/#/c/8472/5/be/src/rpc/impala-service-pool.h@76
PS5, Line 76: resources
what resources?  Let's just remove that word since we've been trying to be very 
clear about what a "resource" is in Impala lately.



--
To view, visit http://gerrit.cloudera.org:8080/8472
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia3e0fa9ae22c4803b0cea736eb35e333e2671448
Gerrit-Change-Number: 8472
Gerrit-PatchSet: 5
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Comment-Date: Mon, 04 Dec 2017 22:54:23 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4671: (part-2) Replace kudu::ServicePool with one that uses Impala threads

2017-12-04 Thread Sailesh Mukil (Code Review) 
Sailesh Mukil has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8472 )

Change subject: IMPALA-4671: (part-2) Replace kudu::ServicePool with one that 
uses Impala threads
..


Patch Set 5:

(5 comments)

http://gerrit.cloudera.org:8080/#/c/8472/4//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/8472/4//COMMIT_MSG@33
PS4, Line 33: This patch however does not display instrumentation on the 
Webpages yet.
: In a future patch, we will add that through the ImpalaServicePool 
and the
: RpcMgr.
> can you file a JIRA for that (and you could reference it here).
Done


http://gerrit.cloudera.org:8080/#/c/8472/4/be/src/rpc/impala-service-pool.h
File be/src/rpc/impala-service-pool.h:

http://gerrit.cloudera.org:8080/#/c/8472/4/be/src/rpc/impala-service-pool.h@65
PS4, Line 65:   // TODO: Display these metrics in the debug webpage. IMPALA-6269
:   // Number of RPCs that timed out while waiting in the service 
queue.
:   AtomicInt32 rpcs_timed_out_in_queue_;
:   // Number of RPCs that were rejected due to the queue being 
full.
:   AtomicInt32 rpcs_queue_overflow_;
> these aren't used by anything yet, right? let's add a TODO with a JIRA numb
Done


http://gerrit.cloudera.org:8080/#/c/8472/4/be/src/rpc/impala-service-pool.h@75
PS4, Line 75:
> comment on what that synchronizes
Done. Since the lock theoretically seems unnecessary, I've left a TODO to 
consider removing it later.


http://gerrit.cloudera.org:8080/#/c/8472/4/be/src/rpc/impala-service-pool.cc
File be/src/rpc/impala-service-pool.cc:

http://gerrit.cloudera.org:8080/#/c/8472/4/be/src/rpc/impala-service-pool.cc@40
PS4, Line 40:
: METRIC_DEFINE_histogram(server, impala_unused,
: "RPC Queue Time",
: kudu::MetricUnit::kMic
> include names.h instead
Done


http://gerrit.cloudera.org:8080/#/c/8472/4/be/src/service/impalad-main.cc
File be/src/service/impalad-main.cc:

http://gerrit.cloudera.org:8080/#/c/8472/4/be/src/service/impalad-main.cc@58
PS4, Line 58:
> why do we have that now?
It's not needed. Removed.



--
To view, visit http://gerrit.cloudera.org:8080/8472
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia3e0fa9ae22c4803b0cea736eb35e333e2671448
Gerrit-Change-Number: 8472
Gerrit-PatchSet: 5
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Comment-Date: Mon, 04 Dec 2017 21:35:20 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4671: (part-2) Replace kudu::ServicePool with one that uses Impala threads

2017-12-04 Thread Sailesh Mukil (Code Review) 
Hello Michael Ho, Lars Volker, Dan Hecht,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/8472

to look at the new patch set (#5).

Change subject: IMPALA-4671: (part-2) Replace kudu::ServicePool with one that 
uses Impala threads
..

IMPALA-4671: (part-2) Replace kudu::ServicePool with one that uses Impala 
threads

The KuduRPC subsystem uses kudu::ServicePool to service all incoming
RPCs. Since this lives inside the Kudu codebase, all instrumentation
of RPCs flowing through the KuduRPC subsystem is not visible to Impala,
as Kudu does not export this instrumentation, but only maintains it
internally within kudu::ServicePool.

In order to reliably view the instrumentation of all RPCs flowing through
KRPC, one option is to modify kudu::ServicePool to take their
instrumentation and display it on our webpages, which is very invasive.

A second option is to have a parallel implementation of kudu::ServicePool
which for all purposes behaves the same way, but instead has this extra
code that displays this instrumentation in our webpages.

This patch is Part 2 of the second option.

In Part 1, we simply copied the code from kudu::ServicePool into the Impala
namespace. In this patch, we rename the redundant kudu::ServicePool
(that was copied into be/src/rpc/) to ImpalaServicePool. We also expose
the instrumentaion of the ImpalaServicePool as JSON.

Now, the threads in use by the service pool will also be visible in
the /threadz page.

This patch however does not display instrumentation on the Webpages yet.
In a future patch, we will add that through the ImpalaServicePool and the
RpcMgr. Tracked by IMPALA-6269

This patch is partially based on an abandoned patch by Henry Robinson.

Change-Id: Ia3e0fa9ae22c4803b0cea736eb35e333e2671448
---
M be/src/rpc/CMakeLists.txt
M be/src/rpc/impala-service-pool.cc
M be/src/rpc/impala-service-pool.h
M be/src/rpc/rpc-mgr.cc
M be/src/rpc/rpc-mgr.h
M be/src/rpc/rpc-trace.h
6 files changed, 116 insertions(+), 138 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/72/8472/5
--
To view, visit http://gerrit.cloudera.org:8080/8472
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ia3e0fa9ae22c4803b0cea736eb35e333e2671448
Gerrit-Change-Number: 8472
Gerrit-PatchSet: 5
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil

[Impala-ASF-CR] IMPALA-4671: (part-2) Replace kudu::ServicePool with one that uses Impala threads

2017-12-04 Thread Dan Hecht (Code Review) 
Dan Hecht has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8472 )

Change subject: IMPALA-4671: (part-2) Replace kudu::ServicePool with one that 
uses Impala threads
..


Patch Set 4:

(5 comments)

What testing did you do? Did you verify the threads show up on /threadz and 
things look right when krpc is enabled?

http://gerrit.cloudera.org:8080/#/c/8472/4//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/8472/4//COMMIT_MSG@33
PS4, Line 33: This patch however does not display instrumentation on the 
Webpages yet.
: In a future patch, we will add that through the ImpalaServicePool 
and the
: RpcMgr.
can you file a JIRA for that (and you could reference it here).


http://gerrit.cloudera.org:8080/#/c/8472/4/be/src/rpc/impala-service-pool.h
File be/src/rpc/impala-service-pool.h:

http://gerrit.cloudera.org:8080/#/c/8472/4/be/src/rpc/impala-service-pool.h@65
PS4, Line 65:   // Number of RPCs that timed out while waiting in the service 
queue.
:   AtomicInt32 rpcs_timed_out_in_queue_;
:
:   // Number of RPCs that were rejected due to the queue being 
full.
:   AtomicInt32 rpcs_queue_overflow_;
these aren't used by anything yet, right? let's add a TODO with a JIRA number. 
Could do the same thing for "unused_histogram_".


http://gerrit.cloudera.org:8080/#/c/8472/4/be/src/rpc/impala-service-pool.h@75
PS4, Line 75:   boost::mutex shutdown_lock_;
comment on what that synchronizes


http://gerrit.cloudera.org:8080/#/c/8472/4/be/src/rpc/impala-service-pool.cc
File be/src/rpc/impala-service-pool.cc:

http://gerrit.cloudera.org:8080/#/c/8472/4/be/src/rpc/impala-service-pool.cc@40
PS4, Line 40: using boost::lock_guard;
: using boost::mutex;
: using std::shared_ptr;
: using strings::Substitute;
include names.h instead


http://gerrit.cloudera.org:8080/#/c/8472/4/be/src/service/impalad-main.cc
File be/src/service/impalad-main.cc:

http://gerrit.cloudera.org:8080/#/c/8472/4/be/src/service/impalad-main.cc@58
PS4, Line 58: DECLARE_bool(use_krpc);
why do we have that now?



--
To view, visit http://gerrit.cloudera.org:8080/8472
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia3e0fa9ae22c4803b0cea736eb35e333e2671448
Gerrit-Change-Number: 8472
Gerrit-PatchSet: 4
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Comment-Date: Mon, 04 Dec 2017 18:26:26 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4671: (part-2) Replace kudu::ServicePool with one that uses Impala threads

2017-12-03 Thread Sailesh Mukil (Code Review) 
Sailesh Mukil has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8472 )

Change subject: IMPALA-4671: (part-2) Replace kudu::ServicePool with one that 
uses Impala threads
..


Patch Set 4:

The latest patchset does away with the webpage instrumentation for KRPC metrics 
and only exposes the Service Pool threads in the /threadz page.


--
To view, visit http://gerrit.cloudera.org:8080/8472
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia3e0fa9ae22c4803b0cea736eb35e333e2671448
Gerrit-Change-Number: 8472
Gerrit-PatchSet: 4
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Comment-Date: Mon, 04 Dec 2017 04:57:13 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4671: (part-2) Replace kudu::ServicePool with one that uses Impala threads

2017-12-02 Thread Sailesh Mukil (Code Review) 
Sailesh Mukil has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8472 )

Change subject: IMPALA-4671: (part-2) Replace kudu::ServicePool with one that 
uses Impala threads
..


Patch Set 4:

(26 comments)

http://gerrit.cloudera.org:8080/#/c/8472/3/be/src/rpc/impala-service-pool.h
File be/src/rpc/impala-service-pool.h:

http://gerrit.cloudera.org:8080/#/c/8472/3/be/src/rpc/impala-service-pool.h@52
PS3, Line 52:   virtual kudu::Status
:   QueueInboundCall(gscoped_ptr call) 
OVERRIDE;
:
:   const std::string service_name() const;
:
:  private:
:   void RunThread();
:
> let's add header function comments to these (and other methods), per usual.
Removed this. N/A


http://gerrit.cloudera.org:8080/#/c/8472/3/be/src/rpc/impala-service-pool.h@69
PS3, Line 69: micInt
> What is this referring to? RPC service methods, or something different?
Removed this. N/A


http://gerrit.cloudera.org:8080/#/c/8472/3/be/src/rpc/impala-service-pool.h@72
PS3, Line 72:   // appropriate internal KRPC state. Unused otherwise.
> what is "time" in these cases? wallclock, cpu, ...?
Removed this. N/A


http://gerrit.cloudera.org:8080/#/c/8472/3/be/src/rpc/impala-service-pool.h@74
PS3, Line 74:
> what's that? some comments here would help.
Removed this. N/A


http://gerrit.cloudera.org:8080/#/c/8472/3/be/src/rpc/impala-service-pool.h@92
PS3, Line 92:
> what does that protect?
Looks like it's left over from a long time ago in the Kudu code, so as far as 
we know it doesn't really protect anything any more.

However, the Kudu folks were skeptical about removing it since it doesn't 
negatively affect anything right now, and we're not sure if there may be some 
issues after removing it.

Let me know what you think.


http://gerrit.cloudera.org:8080/#/c/8472/3/be/src/rpc/impala-service-pool.cc
File be/src/rpc/impala-service-pool.cc:

http://gerrit.cloudera.org:8080/#/c/8472/3/be/src/rpc/impala-service-pool.cc@182
PS3, Line 182: if (PRED
> Hmm, okay I see we are fishing into the timing_ struct and doing math again
Removed this. N/A


http://gerrit.cloudera.org:8080/#/c/8472/3/be/src/rpc/impala-service-pool.cc@186
PS3, Line 186: bably ig
> internal is too vague. Let's "to update the InboundCall timing".  timing()
Done


http://gerrit.cloudera.org:8080/#/c/8472/3/be/src/rpc/impala-service-pool.cc@187
PS3, Line 187:
> I guess you're basically doing this manually via time_in_queue computation.
This is the API that KRPC expects. They expect a Histogram to be supplied to 
update the incoming queue time for all RPCs.

Handler latency is a per RPC histogram whereas the incoming queue time 
histogram is over all RPCs.

In any case, I removed all per RPC histograms, so we're just passing the 
unused_histogram_ to satiate the API requirements now.


http://gerrit.cloudera.org:8080/#/c/8472/3/be/src/rpc/impala-service-pool.cc@212
PS3, Line 212:
> there's no enumeration for the methods? i guess this is okay though.
Removed this. N/A


http://gerrit.cloudera.org:8080/#/c/8472/3/be/src/rpc/impala-service-pool.cc@213
PS3, Line 213:
> is that purposely protected by the lock?
Removed this. N/A


http://gerrit.cloudera.org:8080/#/c/8472/3/be/src/rpc/impala-service-pool.cc@220
PS3, Line 220:
> this is a bit decieving becuase we haven't necessarily actually handled the
Removed this. N/A


http://gerrit.cloudera.org:8080/#/c/8472/3/be/src/rpc/impala-service-pool.cc@223
PS3, Line 223:
> why not just do that?
Removed this. N/A


http://gerrit.cloudera.org:8080/#/c/8472/3/be/src/rpc/impala-service-pool.cc@240
PS3, Line 240:
> doesn't krpc already have these histograms? is all we want to get out is th
I have a working implementation of getting the histograms from Kudu and using 
it, however, since we decided to defer it, I'm not including it in this patch.

Just to leave a note though, Kudu has the handling time histogram that we need 
to get from them since we cannot accurately calculate that ourselves. They 
payload size histogram and the per RPC queueing time histogram can still be 
calculated by us over here when we decide to do it.

There are a few other interesting metrics like "total number of accepted 
connections", etc. that we could display.


http://gerrit.cloudera.org:8080/#/c/8472/3/be/src/rpc/impala-service-pool.cc@255
PS3, Line 255:
> should we also display the total number of service threads even though it's
That's already displayed in the threadz/ page automatically, since all these 
threads are Impala threads.


http://gerrit.cloudera.org:8080/#/c/8472/3/be/src/rpc/rpc-mgr.h
File be/src/rpc/rpc-mgr.h:

http://gerrit.cloudera.org:8080/#/c/8472/3/be/src/rpc/rpc-mgr.h@a160
PS3, Line 160:
> is that not still true? i.e. since we need to inherit from kudu::RpcService
It actually should be the same. Not sure why I removed that comment. I put it 
back in.

[Impala-ASF-CR] IMPALA-4671: (part-2) Replace kudu::ServicePool with one that uses Impala threads

2017-12-02 Thread Sailesh Mukil (Code Review) 
Hello Michael Ho, Lars Volker, Dan Hecht,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/8472

to look at the new patch set (#4).

Change subject: IMPALA-4671: (part-2) Replace kudu::ServicePool with one that 
uses Impala threads
..

IMPALA-4671: (part-2) Replace kudu::ServicePool with one that uses Impala 
threads

The KuduRPC subsystem uses kudu::ServicePool to service all incoming
RPCs. Since this lives inside the Kudu codebase, all instrumentation
of RPCs flowing through the KuduRPC subsystem is not visible to Impala,
as Kudu does not export this instrumentation, but only maintains it
internally within kudu::ServicePool.

In order to reliably view the instrumentation of all RPCs flowing through
KRPC, one option is to modify kudu::ServicePool to take their
instrumentation and display it on our webpages, which is very invasive.

A second option is to have a parallel implementation of kudu::ServicePool
which for all purposes behaves the same way, but instead has this extra
code that displays this instrumentation in our webpages.

This patch is Part 2 of the second option.

In Part 1, we simply copied the code from kudu::ServicePool into the Impala
namespace. In this patch, we rename the redundant kudu::ServicePool
(that was copied into be/src/rpc/) to ImpalaServicePool. We also expose
the instrumentaion of the ImpalaServicePool as JSON.

Now, the threads in use by the service pool will also be visible in
the /threadz page.

This patch however does not display instrumentation on the Webpages yet.
In a future patch, we will add that through the ImpalaServicePool and the
RpcMgr.

This patch is partially based on an abandoned patch by Henry Robinson.

Change-Id: Ia3e0fa9ae22c4803b0cea736eb35e333e2671448
---
M be/src/rpc/CMakeLists.txt
M be/src/rpc/impala-service-pool.cc
M be/src/rpc/impala-service-pool.h
M be/src/rpc/rpc-mgr.cc
M be/src/rpc/rpc-mgr.h
M be/src/rpc/rpc-trace.h
M be/src/service/impalad-main.cc
7 files changed, 116 insertions(+), 136 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/72/8472/4
--
To view, visit http://gerrit.cloudera.org:8080/8472
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ia3e0fa9ae22c4803b0cea736eb35e333e2671448
Gerrit-Change-Number: 8472
Gerrit-PatchSet: 4
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Michael Ho