Thomas Tauber-Marshall has posted comments on this change.

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


Patch Set 4:

(10 comments)

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

PS3, Line 204: 
> this is an important enough parameter that I'd make a constant for it, and 
Done


PS3, Line 216: 
> If this returns false it's because the queue is shut down. Better to say th
Done


PS3, Line 218: break;
> set stop_ to true to cleanup?
Done


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

Line 46:  *
> add a reference to IMPALA-4135 here.
Done


PS3, Line 105: 
> Is this used anywhere?
Done


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

Line 174:   // Test that a large number of concurrent connections will all 
succeed and not time out
> Add a comment about what you're testing and the associated JIRA.
Done


PS3, Line 178: _OK(server->Start(
> Does this work in a threaded context - does the test fail if the thread hit
Yes, although the test runs to completion before reporting the error.


PS3, Line 180: ThreadPool<int64_t> pool(
> I'm a bit concerned about failing with ulimit errors, particularly because 
Agreed, this test has a lot of potential for flakiness or other pain. Not sure 
how else to do an automatic test of this issue, though.

As written, it doesn't even actually fail on my machine without the fix, unless 
I add an artificial slowdown.


PS3, Line 173: TEST(ConcurrencyTest, ConnectTimeout) {
             :   // Test that a large number of concurrent connections will all 
succeed and not time out
             :   // waiting to be accepted.(IMPALA-4135)
             :   int port = GetServerPort();
             :   ThriftServer* server = new ThriftServer("DummyServer", 
MakeProcessor(), port);
             :   ASSERT_OK(server->Start());
             : 
             :   ThreadPool<int64_t> pool(
             :       "group", "test", 256, 10000, [port](int tid, const 
int64_t& item) {
             :  
> This relies on a running impala internal service - you probably had an Impa
Done


http://gerrit.cloudera.org:8080/#/c/4519/1/be/src/server/TAcceptQueueThreadedServer.cpp
File be/src/server/TAcceptQueueThreadedServer.cpp:

PS1, Line 233: 
> Oops, sorry about that.
No problem.


-- 
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