[
https://issues.apache.org/jira/browse/THRIFT-3084?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14490722#comment-14490722
]
Randy Abernethy commented on THRIFT-3084:
-----------------------------------------
Hey James:
I have a different view on the implementation inheritance front.
TSimpleServer has a responsibility, be simple, be a good way to understand
thrift server basics and provide a clean, simple, single threaded
implementation. TThreadPoolServer has a very different purpose, provide a
thread per client based server implementation which can be used for handling
scaled production loads. Semantically there is no implied implementation bond
here, only the fact that they are both Apache Thrift servers.
TSimpleServer should have the TServer interface, but should it have the same
implementation as TThreadPoolServer? It mostly does now but will it always? Is
it easier or harder for me to understand the simple server with code in various
classes handling the implementation? What if someone decides to add an
IOCompletion port server for Windows? Should it use the framework
implementation designed for TThreadPoolServer? Can the NonblockingServer? What
if I need to update the TServerFramework implementation for TThreadPoolServer?
I would need to be an expert in all of the derived servers to know for sure
that my implementation changes will work with each of them. At this point
TSimpleServer is not very simple, it is no longer performing its one
responsibility.
Tightly coupled classes (and systems in general) make maintenance hard. There
is no tighter way to couple classes than implementation inheritance. There are
some fairly well thought out guidelines that relate to this issue, for example:
Alexandrescu, Andrei; Herb Sutter (2004-10-25). C++ Coding Standards: 101
Rules, Guidelines, and Best Practices
- 34. Prefer composition to inheritance
- 36. Prefer providing abstract interfaces
- 37. Public inheritance is substitutability. Inherit, not to reuse, but to be
reused
I have seen many crimes against encapsulation, separation of concerns and KIS
committed in the name of DRY.
I am all for the other code improvements, particularly the TThreadPoolServer
code you have written in the TServerFramework class. My “keep it simple”
approach would be to integrate the TThreadPoolServer fixes standalone,
deprecate TThreadedServer (implementing it with TThreadPoolServer as Ben
suggests) and keep TSimpleServer simple and self-contained.
Just my 2 cents and certainly subjective. You are making things better patch by
patch so I am a fan either way.
Best,
Randy
> C++ add concurrent client limit to threaded servers
> ---------------------------------------------------
>
> Key: THRIFT-3084
> URL: https://issues.apache.org/jira/browse/THRIFT-3084
> Project: Thrift
> Issue Type: Improvement
> Components: C++ - Library
> Affects Versions: 0.8, 0.9, 0.9.1, 0.9.2
> Reporter: James E. King, III
>
> The TThreadedServer and TThreadPoolServer do not impose limits on the number
> of simultaneous connections, which is not useful in production as bad clients
> can drive a server to consume too many file descriptors or have too many
> threads.
> 1. Add a barrier to TServerTransport that will be checked before accept().
> 2. In the onClientConnected override (see THRIFT-3083) if the server reaches
> the limit of the number of accepted clients, enable the barrier.
> 3. In the onClientDisconnected override if the count of connected clients
> falls below the maximum concurrent limit, clear the barrier. This will allow
> the limit to be changed dynamically at runtime (lowered) with drain off
> clients until more can be accepted.
> Alternate proposal: Implement a Semaphore and have the servers block the
> serve() thread if the client that arrived puts the server at the concurrent
> client limit.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
