[
https://issues.apache.org/jira/browse/THRIFT-2398?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13932808#comment-13932808
]
Randy Abernethy commented on THRIFT-2398:
-----------------------------------------
Hi Pierre,
Thanks for the comments. I agree, it would be nice to have a conventional
Apache Thrift set of end point transports and layering functionality. At
present we have a relationship something like:
- createConnection() == TSocket (or TSSLSocket is if using TLS)
- createServer() == TServerSocket (or TSSLServerSocket if using TLS)
It takes createClient() combined with createConnection() to build something
akin to the typical "new Client" assembly and the I/O stack has a rigid
definition (only 1 layered transport supported). On the server side the event
driven nature of Node has trivialized the TXXSevers out of existence.
TSimpleServer boils down to a processor.process() read loop in most languages,
which there is no need for in the face of the 'data' event. In essence, Node.js
== TSimpleServer.
In Node.js we also have no file or memory end point transports (e.g.
TFileTransport and TMemoryBuffer (which should be TMemoryTransport, because it
is)). Another shortfall is the lack of a TCompactProtocol. On a total tangent,
I also think we should have an Apache Thrift wide VERSION_X of TBinaryProtocol
that can transmit little endian if it is native on both sides, falling back to
VERSION_1 otherwise (saw Ben mention this the other day as well).
On the bright side using the create idiom is safer than relying on the user to
call "new" in JavaScript and it is an idiom immediately recognizable to a
Node.js programmer. Maybe thrift.createTSocket() makes sense. I'm not sure
where convention ends and dogma begins in this regard. Node.js trivializes many
end point tasks that take a lot of code in C++/Java. We don't want to loose the
elegance that Node.js offers.
I do particularly miss the ability to use more than one layered transport. For
example, in C++ you can (assume all the make_shared & shared_ptrs):
{code:title=I/O Stack|borderStyle=solid}
TTransport ep_trans1 = new TSocket(localhost, 5050);
TTransport ep_trans2 = new TSimpleFileTransport(/tmp/blah);
TTransport l1_trans = new TFramedTransport(ep_trans1);
TTransport mux_trans = new TMyMux(l1_trans, ep_trans2);
TBinaryAutoEndianProtocol proto = new TBinaryAutoEndianProtocol(mux_trans);
{code}
To some degree we need to let each language play to its strengths.
Interoperability is a must, but after that when do we favor Thrift convention
and when do language idioms make more sense? Maybe we need an imperative set of
guidelines and a functional set of guidelines. This could tie in with do Scala
justice (the imperative approach is no doubt one of the reasons Scrooge and
Finagle exist). Would be interesting to hear what folks think is the right
idiomatic answer for Apache Thrift in Node.js given a blank canvas.
Anyway, sounds like you agree with the need for createServer and options clean
up. Let me know if not, otherwise I will post a patch which makes these
incremental improvements:
- documenting and centralizing all of the createConnection() and
createServer() options
- refining and documenting the server API to:
var server = thrift.createServer(processor, handler, options);
var server = thrift.createMultiplexServer(processor, options);
var server = thrift.createWebServer(options);
- making the presence of options.tlsOptions add an SLL/TLS to all three servers
(without any loss of functionality or breaks in
createServer()/createMultiplexServer() backward compatibility of course)
I will also update all of the tests, kudos to Roger and co for the awesome work
on the test front!
Very best,
Randy
> 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)
