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 <sail...@cloudera.com>
Gerrit-Reviewer: Michael Ho <k...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sail...@cloudera.com>
Gerrit-Comment-Date: Fri, 02 Feb 2018 23:46:14 +0000
Gerrit-HasComments: Yes

Reply via email to