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 <[email protected]> Gerrit-Reviewer: Henry Robinson <[email protected]> Gerrit-Reviewer: Juan Yu <[email protected]> Gerrit-Reviewer: Matthew Jacobs <[email protected]> Gerrit-Reviewer: Sailesh Mukil <[email protected]> Gerrit-Reviewer: Thomas Tauber-Marshall <[email protected]> Gerrit-HasComments: Yes
