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 <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
