Henry Robinson has posted comments on this change.

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

Patch Set 1:


I think Sailesh's suggestion is a good one. Is there any state in 
TThreadedServer that can't be accessed by a subclass? 

 I agree that a separate library seems unnecessary. There doesn't seem any need 
for a separate top-level dir; I expected a new directory under rpc/. But the 
simplest is probably just to put the file in rpc/ and be done with it.

File be/src/rpc/thrift-server.cc:

Line 419:       server_.reset(new TAcceptQueueThreadedServer(processor_, 
let's make the choice of server configurable by a flag that we can turn off if 

File be/src/server/TAcceptQueueThreadedServer.cpp:

Line 144:     inputTransportFactory_->getTransport(client);
do you see much slowdown in synchronizing around these few lines that create 
protocols, transports and processors? If not, let's do that to make sure we 
rule out any possibility of race conditions.

Line 164:     shared_ptr<Thread> thread = 
Is this definitely thread-safe? I think this might be the bottleneck (or maybe 
it's start()?) so it's going to be useful to keep unsynchronized.

Line 200:   ThreadPool<shared_ptr<TTransport>> acceptPool("server-accept", 
"accept-worker", 1, 10000,
How did you arrive at 1 as the default number of worker threads? What happens 
as that number increases? If it *has* to be one, to keep doAccept() 
thread-safe, add a comment here to that effect.

Line 236:       acceptPool.DrainAndShutdown();
I'm not sure we ever actually stop the thrift servers, so it might be moot - 
but shouldn't you signal to the worker threads that they should shut down and 
therefore not create any new threads?

File be/src/server/TAcceptQueueThreadedServer.h:

PS1, Line 1: // This file was copied from 
apache::thrift::server::TThreadedServer.cpp v0.9.0, with the
           : // significant changes noted inline below.
           : /*
           :  * Licensed to the Apache Software Foundation (ASF) under one
           :  * or more contributor license agreements. See the NOTICE file
           :  * distributed with this work for additional information
           :  * regarding copyright ownership. The ASF licenses this file
           :  * to you under the Apache License, Version 2.0 (the
           :  * "License"); you may not use this file except in compliance
           :  * with the License. You may obtain a copy of the License at
           :  *
           :  *   http://www.apache.org/licenses/LICENSE-2.0
           :  *
           :  * Unless required by applicable law or agreed to in writing,
           :  * software distributed under the License is distributed on an
           :  * KIND, either express or implied. See the License for the
           :  * specific language governing permissions and limitations
           :  * under the License.
           :  */
> I think this is fine, but if you haven't already, can you ask Jim or Henry 
AFAIK it's completely fine - copying from another ASF project is legit.

That said, I'd keep the license header as the first thing in the file (and if 
you subclass, so much the better).

PS1, Line 44: and effectively creating a new,
            :  * internally managed, accept queue.
not exactly - the connections have been accepted, the next state is transport 

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: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall <tmarsh...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <m...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sail...@cloudera.com>
Gerrit-HasComments: Yes

Reply via email to