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.
Line 419: server_.reset(new TAcceptQueueThreadedServer(processor_,
let's make the choice of server configurable by a flag that we can turn off if
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?
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
To view, visit http://gerrit.cloudera.org:8080/4519
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
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>