[Impala-ASF-CR] IMPALA-5394: Change ThriftServer() to always use TAcceptQueueServer

["Sailesh Mukil (Code Review)" <ge...@cloudera.org>]

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 <jf...@arcadiadata.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: John Sherman <jf...@arcadiadata.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@apache.org>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Comment-Date: Fri, 29 Sep 2017 23:45:03 +0000
Gerrit-HasComments: Yes