[Impala-ASF-CR] IMPALA-4671: (part-2) Replace kudu::ServicePool with one that uses Impala threads
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 MukilGerrit-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
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 MukilTested-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
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 MukilGerrit-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
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 MukilGerrit-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
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 MukilGerrit-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
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 MukilGerrit-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
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 MukilGerrit-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
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 MukilGerrit-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
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 MukilGerrit-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
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 MukilGerrit-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
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 MukilGerrit-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
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 MukilGerrit-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
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 MukilGerrit-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
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
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 MukilGerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Michael Ho