[
https://issues.apache.org/jira/browse/THRIFT-2398?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13932471#comment-13932471
]
Pierre Lamot commented on THRIFT-2398:
--------------------------------------
Hi,
While I agree that we can (should) re-factor the server interface, I'm not
really sure about the way you propose.
I think that we should try to be more consistent with the API proposed in the
other languages. Most other languages (C++, python, C#, D, go, …) propose
something like
{quote}
...
transport = MyTransport()
transportFactory = MyTransportFactory()
server = MyServer(processor, transport, transportFactory, protocolFactory)
server.serve()
{quote}
where MyTransport is TServerSocket, TSSLServerSocket, THttpServer, … so the
kind of transport is explicit and not implied by options (in the case of SSL).
The createXXXserver had the benefit of keeping things explicit and _a bit_ like
the other language but it merge the concept of transport and server which is
bad, to my mind.
So, ideally I would:
- make each transport as its own class, with the same naming scheme as in the
other language
- add a TSimpleServer class (same behavior as the other languages)
- keep createServer/createMultiplexedServer for backward compatibility with
0.9.1, mark them as deprecated
regarding tls/https options, I don't really care, I did it that way because
options didn't overlapped (I find it more practical to have only one float
options map), but i agree that it can become a problem if we (or node
tls/https) require new options
cheers,
Pierre
> Improve Node Server Library
> ---------------------------
>
> Key: THRIFT-2398
> URL: https://issues.apache.org/jira/browse/THRIFT-2398
> Project: Thrift
> Issue Type: Improvement
> Components: Node.js - Library
> Affects Versions: 0.9.2
> Environment: all
> Reporter: Randy Abernethy
>
> Improve Node Server Library
> =======================
> Background
> ----------------
> In the last 7 months Node.js has gone from one server constructor:
> • createServer()
> to seven, adding:
> • createSSLServer()
> • createMultiplexServer()
> • createMultiplexSSLServer()
> • createHttpServer()
> • createHttpsSSLServer()
> • createWebServer()
> there are really only two implementations:
> • createMultiplexSSLServer()
> • createWebServer()
> Several of these servers have undocumented options and share options objects
> with other libraries.
> Proposal
> -------------
> 1. Remove all of the create signatures except these three:
> o createServer()
> o createMultiplexServer()
> o createWebServer()
> with createServer() implemented by createMultiplexServer(). All signatures
> will support optional multiplexing and optional SSL/TLS. Eliminate no present
> functionality and maintain signature compatibility in the three signatures
> preserved.
> 2. Document and standardize all server options and parameters with notes
> describing any deprecated features being preserved for backward compatibility.
> Motivation
> -------------
> The dispersion of create methods makes it harder for developers to know which
> server to use and leads to diffusion in the way that options and features are
> provided. This also complicates testing. Reducing the servers to the two
> currently supported end point transports (TCP and HTTP) will enforce
> standardization and simplify adoption. Now is the time to address these
> issues before the new create signatures show up in a released version.
> Approach to Options
> ----------------------------
> Presently the non-web server options objects may have transport and protocol
> properties. Undocumented key and cert properties are used to enable the
> options object to be passed to the Node.js tls and https createServer()
> methods. This approach requires Apache Thrift options to be visible to the
> Node.js library methods and vice versa. A better approach might be to place
> Node.js tls options in a tlsOptions object which is itself a property of the
> server options object. This will allow any tlsOptions to be passed through to
> Node.js without concerns for conflicts on the Node.js or Apache Thrift side,
> now or in the future. The presence of a tlsOptions object can also be used to
> enable SSL/TLS in the two server implementations rather than having separate
> functions.
> Would like to know what others think about this.
--
This message was sent by Atlassian JIRA
(v6.2#6252)
