Henry Robinson has posted comments on this change.

Change subject: IMPALA-4135: Thrift threaded server times-out connections 
during high load
......................................................................


Patch Set 4:

(7 comments)

Looking pretty good.

http://gerrit.cloudera.org:8080/#/c/4519/4/be/src/rpc/TAcceptQueueServer.cpp
File be/src/rpc/TAcceptQueueServer.cpp:

PS4, Line 136: doAccept
doAccept() isn't a great name (I know it was from my patch!) because this isn't 
actually doing the work of accept, but instead establishing a connection. 
Perhaps call this SetupConnection() ?

You can use Impala's convention for method capitalisation; again makes it 
easier to see what was added.


PS4, Line 204: acceptPool
connection_handler_pool, or something similar.


PS4, Line 205: ACCEPT_THREAD_POOL_SIZE
This can be defined in this method as a constexpr.


http://gerrit.cloudera.org:8080/#/c/4519/4/be/src/rpc/TAcceptQueueServer.h
File be/src/rpc/TAcceptQueueServer.h:

PS4, Line 96: thread
strange line breaks in this comment.


PS4, Line 97: static const int ACCEPT_THREAD_POOL_SIZE = 1;
move into .cc file


http://gerrit.cloudera.org:8080/#/c/4519/4/be/src/rpc/thrift-server-test.cc
File be/src/rpc/thrift-server-test.cc:

PS4, Line 173: ConnectTimeout
Call this ManyConcurrentConnection or something.


Line 175:   // waiting to be accepted.(IMPALA-4135)
Mention that this does not always fail. How long does it take to run?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie50e728974ef31a9d49132a0b3f7cde2a4f3356d
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall <tmarsh...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Juan Yu <j...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <m...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sail...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tmarsh...@cloudera.com>
Gerrit-HasComments: Yes

Reply via email to