Sailesh Mukil has posted comments on this change. ( http://gerrit.cloudera.org:8080/9186 )
Change subject: IMPALA-6456: Add flags to configure rpc_negotiation_timeout_ms and negotiation thread count in KRPC ...................................................................... Patch Set 1: (4 comments) http://gerrit.cloudera.org:8080/#/c/9186/1/be/src/rpc/rpc-mgr.cc File be/src/rpc/rpc-mgr.cc: http://gerrit.cloudera.org:8080/#/c/9186/1/be/src/rpc/rpc-mgr.cc@70 PS1, Line 70: Number of threads to dedicate to process connection negotiations > Maximum number of threads dedicated to handling RPC connection negotiations Done http://gerrit.cloudera.org:8080/#/c/9186/1/be/src/rpc/rpc-mgr.cc@84 PS1, Line 84: bld.set_rpc_negotiation_timeout_ms(FLAGS_rpc_negotiation_timeout_ms); > Does it make sense to also call bld.set_min_negotiation_threads(1) ? The min number of threads is 0 by default. What that means is that there won't be any "permanent" thread that is always waiting to do negotiation. They will be spawned as required (up to max_thread_count) and live for some small period after which it times out and kills itself. Given our workload, we would have bursts of negotiations at different points in time with potentially vast intervals in between. So maybe it's better to save the resources of that one thread. What do you think? http://gerrit.cloudera.org:8080/#/c/9186/1/be/src/rpc/rpc-mgr.cc@84 PS1, Line 84: FLAGS_rpc_negotiation_timeout_ms > What happens if this flag is set to negative by accident ? Will the code cr There's a CHECK to make sure that this isn't negative: https://github.com/apache/impala/blob/master/be/src/kudu/util/threadpool.cc#L79 So it will cause a crash. http://gerrit.cloudera.org:8080/#/c/9186/1/be/src/rpc/rpc-mgr.cc@85 PS1, Line 85: FLAGS_rpc_negotiation_thread_count > Will it be safer to do max(1, FLAGS_rpc_negotiation_thread_count) to guaran Yes that makes sense. Done. -- To view, visit http://gerrit.cloudera.org:8080/9186 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I108d700e7eac04b678e21a3a920aac81ba8eede5 Gerrit-Change-Number: 9186 Gerrit-PatchSet: 1 Gerrit-Owner: Sailesh Mukil <[email protected]> Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Reviewer: Sailesh Mukil <[email protected]> Gerrit-Comment-Date: Fri, 02 Feb 2018 23:46:14 +0000 Gerrit-HasComments: Yes
