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:

File be/src/rpc/TAcceptQueueServer.cpp:

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

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

PS3, Line 218: break;
> set stop_ to true to cleanup?
File be/src/rpc/TAcceptQueueServer.h:

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

PS3, Line 105: 
> Is this used anywhere?
File be/src/rpc/

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.

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
File be/src/server/TAcceptQueueThreadedServer.cpp:

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

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie50e728974ef31a9d49132a0b3f7cde2a4f3356d
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall <>
Gerrit-Reviewer: Henry Robinson <>
Gerrit-Reviewer: Juan Yu <>
Gerrit-Reviewer: Matthew Jacobs <>
Gerrit-Reviewer: Sailesh Mukil <>
Gerrit-Reviewer: Thomas Tauber-Marshall <>
Gerrit-HasComments: Yes

Reply via email to