[
https://issues.apache.org/jira/browse/THRIFT-1031?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13105746#comment-13105746
]
alexandre parenteau commented on THRIFT-1031:
---------------------------------------------
@James: I reviewed the patch, and have those remarks to share:
Overall:
========
Pros:
- This is a nice improvement to have a 0.8 patch for Windows
- Almost everything in thrift is ported (however see also Cons)
- IMHO this could be *the* base patch for the upcoming Windows support inside
thrift
Cons:
- the server part is missing (as you mentioned), and/or especially async
servers is incomplete (libevent and pthread based, very important for
high-performance thrift serving)
- the new socket-windows-only code is troubling to me (see below for more
details, and possible alternatives)
- it lacks a 64 bits target (should be trivial to add)
Details:
========
Those observations come mostly from my own experiment of maintaining a Windows
port: see
https://github.com/aubonbeurre/thrift/blob/winthriftnb-0.8.x-1/README.non.blocking.Windows
My focus has been on supporting async server/client, so in a way I feel it is
very complimentary to this patch.
I was hoping you could review my observations, and see if somehow you could
blend the two together!
TSocket:
- It is not immediately obvious to me why creating a different socket class for
Windows is necessary. On the github link above, TSocket has only a couple of
minor changes, and run/compile fine on Windows.
- HOWEVER: Much more importantly, I don't think it will work for multi-thread
servers to use errno (see
http://msdn.microsoft.com/en-us/library/ms737828(v=vs.85).aspx and
http://msdn.microsoft.com/en-us/library/windows/apps/ms737828(v=vs.85).aspx)
- My own approach was to re-define errno to WSAGetLastError (only internally to
thrift), and all the errno.h codes: It works as long as errno is not getting
used inside any thrift .h (or it would force clients linking against thrift to
do the same).
- So taking together those approaches (slight changes to the existing TSocket,
and override of errno) seem better to me, even if they require special
attention to never use errno inside .h. Also errors getting thrown by thrift
have now the WinSock error codes, and it works in multiple threads (like the
pool thread).
lib\cpp\src\concurrency\PosixThreadFactory.cpp:
- In the port mentioned above, you'll find the minor tweaks to have it compile
and run on Windows (it was taken from another JIRA patch, perhaps even this
one!)
lib\cpp\src\server\TNonblockingServer.cpp:
Lately Roger integrated a set of patches of mine which make the compilation on
Windows happen without any changes. However see in the code above the file
win32-config.h, which defines SOCKOPT_CAST_T and AF_LOCAL to make this work.
TWinsockSingleton:
- I'm not sure this is necessary to have thrift initialize WinSock (especially
since libevent might already do this, dunno). I choose to make it a
documentation point.
Thanks, I feel overall the windows port is getting closer and closer.
> Patch to compile Thrift for vc++ 9.0 and 10.0
> ---------------------------------------------
>
> Key: THRIFT-1031
> URL: https://issues.apache.org/jira/browse/THRIFT-1031
> Project: Thrift
> Issue Type: Improvement
> Components: C++ - Library
> Environment: Windows XP 32bit, vc++ 9.0, 10.0
> Reporter: James Dickson
> Priority: Trivial
> Fix For: 0.8
>
> Attachments: thrift_msvc.patch, thrift_msvc_v0_1.patch,
> thrift_msvc_v0_2.patch, thrift_msvc_v0_3.patch, thrift_msvc_v0_4.patch,
> thrift_msvc_v0_5.patch
>
>
> At our company we need clients running on Windows being able to connect to
> our linux servers running hypertable. The attached patch enables the parts
> needed by Hypertable to be compiled on Windows using either the VC++ 9.0 or
> 10.0 compilers.
> Having read previous posts about ports using boost::asio we found these to be
> too intrusive for our needs. This version uses pthreads_win32 and winsock2
> and is as designed to be as un-intrusive as is possible to the original unix
> code base. It is mostly #defines between unix sockets and winsock2 sockets.
> We also tried to follow the folder structuring of the C# runtime that has
> visual studio solutions to be consistent.
> More details are in the README as not all the functionality of the original
> unix code base is available to windows users. We will add the missing
> functionality, we just wanted to share what we had as for a Windows based
> client for us it is sufficient.
> The patch is based on the latest revision in SVN, we would love feedback and
> any code reviews. If there is any possibility of this being added to the main
> trunk then that would be much appreciated, however we don't expect that.
--
This message is automatically generated by JIRA.
For more information on JIRA, see: http://www.atlassian.com/software/jira
