[
https://issues.apache.org/jira/browse/THRIFT-2398?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13938319#comment-13938319
]
Pierre Lamot commented on THRIFT-2398:
--------------------------------------
Hi,
Sorry for the late response, I was out for a couple of days:
The patch looks good to me but there is stills some things that bugs me:
* the createXXXServer returns an objet on which we call listen(port) to start
it. This object is the actual object returned by
(net/tls/http/https).createServer, this is an object from the nodejs core on
which we have no control on its interface and might evolve against our will (I
admit this is unlikely), on an other side the port parameter given to the
listen methods might be irrelevant for an other kind of transport (a memory
transport for instance, I would have said unix socket too, but they do allow to
give a socket path for the port parameter ^_^), for me the port parameter
should be an option of the transport
* createServer/createSSLServer: I have mixed feelings about this, because:
** many languages do the distinction
** the distinction does exists in the node core library, different modules
whether the ssl layer is used or not (http <=> https, net <=> tls)
** ssl might not be present in some future transport (I don't really know if it
makes sense, just thinking of it)
** However I must admit that I rather like to have ssl as an option than a
different class, but I want to be sure that we make a good choice.
* createServer/createConnection: It feel misleading to me that theses functions
are about socket servers/client. A createSocketServer/CreateSocketConnection
seems more explicit to me.
In the end most of theses remarks are mainly a matter of taxonomy but I believe
it has its importance since the choice we made will require to be maintained in
futures version of thrift.
Best regards,
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
> Assignee: Randy Abernethy
> Attachments: 0001-nodejs-server-and-option-cleanup.patch,
> 0001-updated-nodejs-hello-example.patch
>
>
> 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)
