impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Henry Robinson (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-4135: Thrift threaded server times-out connections during high load
Date Mon, 26 Sep 2016 05:03:45 GMT
Henry Robinson has posted comments on this change.

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


Patch Set 1:

(7 comments)

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.

http://gerrit.cloudera.org:8080/#/c/4519/1/be/src/rpc/thrift-server.cc
File be/src/rpc/thrift-server.cc:

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


http://gerrit.cloudera.org:8080/#/c/4519/1/be/src/server/TAcceptQueueThreadedServer.cpp
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 = shared_ptr<Thread>(threadFactory_->newThread(runnable));
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?


http://gerrit.cloudera.org:8080/#/c/4519/1/be/src/server/TAcceptQueueThreadedServer.h
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
           :  * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
           :  * 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 set-up.


-- 
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 <tmarshall@cloudera.com>
Gerrit-Reviewer: Henry Robinson <henry@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sailesh@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message