Sailesh Mukil has posted comments on this change. ( http://gerrit.cloudera.org:8080/7061 )
Change subject: IMPALA-5394: Change ThriftServer() to always use TAcceptQueueServer ...................................................................... Patch Set 8: (3 comments) Sorry for the slow response and thanks for your patience, John. Henry is away on holiday, but let's try to get this in in the meantime. The patch looks good to me. I just have a couple small comments. And rebasing the patch before posting the next patch set would be a good idea. http://gerrit.cloudera.org:8080/#/c/7061/8/be/src/rpc/thrift-server-test.cc File be/src/rpc/thrift-server-test.cc: http://gerrit.cloudera.org:8080/#/c/7061/8/be/src/rpc/thrift-server-test.cc@403 PS8, Line 403: TEST(ConcurrencyTest, MaxConcurrentConnections) { Could you write a brief comment about what this test is trying to achieve? http://gerrit.cloudera.org:8080/#/c/7061/8/be/src/rpc/thrift-server-test.cc@439 PS8, Line 439: EXPECT_TRUE(did_reach_max); > Not sure if this will be effective. Could end up being racy? If so probably It looks like it could be racy, but very rarely. We have no control over how the OS would schedule these threads. We can revisit this test later if it shows up to be a problem, but for now, I'm okay with it. http://gerrit.cloudera.org:8080/#/c/7061/3/be/src/transport/TSaslServerTransport.cpp File be/src/transport/TSaslServerTransport.cpp: http://gerrit.cloudera.org:8080/#/c/7061/3/be/src/transport/TSaslServerTransport.cpp@164 PS3, Line 164: new TSaslServerTransport(serverDefinitionMap_, trans)); It would be good to add the comment here acknowledging the "logical" race, i.e. we know a logical race exists but it won't ever happen because getTransport() won't be called concurrently from different threads with the same 'trans' object. -- To view, visit http://gerrit.cloudera.org:8080/7061 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I56a5f3d9cf931cff14eae7f236fea018236a6255 Gerrit-Change-Number: 7061 Gerrit-PatchSet: 8 Gerrit-Owner: John Sherman <j...@arcadiadata.com> Gerrit-Reviewer: Dan Hecht <dhe...@cloudera.com> Gerrit-Reviewer: Henry Robinson <he...@cloudera.com> Gerrit-Reviewer: John Sherman <j...@arcadiadata.com> Gerrit-Reviewer: Matthew Jacobs <mjac...@apache.org> Gerrit-Reviewer: Sailesh Mukil <sail...@cloudera.com> Gerrit-Comment-Date: Fri, 29 Sep 2017 23:45:03 +0000 Gerrit-HasComments: Yes
