Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: None
    • Labels:
      None
    • Patch Info:
      Patch Available

      Description

      This patch adds support for Twisted in both the compiler and the Python library. Also attached are a client and a server based on the calculator example.

      1. client_20080124.py
        2 kB
        Esteve Fernandez
      2. server_20080124.py
        2 kB
        Esteve Fernandez
      3. thrift-148_v10_20090303_lib.patch
        5 kB
        Esteve Fernandez
      4. thrift-148_v2_20081115.patch
        22 kB
        Esteve Fernandez
      5. thrift-148_v3_20081230.patch
        28 kB
        Esteve Fernandez
      6. thrift-148_v3.1_20081116.patch
        24 kB
        Dan Di Spaltro
      7. thrift-148_v4_20090119.patch
        28 kB
        Esteve Fernandez
      8. thrift-148_v5_20090127.patch
        28 kB
        Esteve Fernandez
      9. thrift-148_v5.1_20090128.patch
        24 kB
        Dan Di Spaltro
      10. thrift-148_v6_20090207.patch
        28 kB
        Esteve Fernandez
      11. thrift-148_v7_20090207.patch
        28 kB
        Esteve Fernandez
      12. thrift-148_v8_20090208.patch
        28 kB
        Esteve Fernandez
      13. thrift-148_v9_20090302.patch
        29 kB
        Esteve Fernandez
      14. txthrift.patch
        21 kB
        Esteve Fernandez

        Activity

        Hide
        Esteve Fernandez added a comment -

        Thanks!

        Show
        Esteve Fernandez added a comment - Thanks!
        Hide
        Kevin Clark added a comment -

        Ok, check rev 749795 for me. Looked ok.

        Show
        Kevin Clark added a comment - Ok, check rev 749795 for me. Looked ok.
        Hide
        Esteve Fernandez added a comment -

        Argh, it looks like thrift-148_v9_20090302.patch was broken, I forgot to add a \ and self.started = defer.Deferred() in ThriftClientProtocol. Those changes are present in thrift-148_v10_20090303_lib.patch, could you reapply the that patch? Sorry about that.

        Show
        Esteve Fernandez added a comment - Argh, it looks like thrift-148_v9_20090302.patch was broken, I forgot to add a \ and self.started = defer.Deferred() in ThriftClientProtocol. Those changes are present in thrift-148_v10_20090303_lib.patch, could you reapply the that patch? Sorry about that.
        Hide
        Kevin Clark added a comment -

        Oops, sorry about that. Was sitting around un-added in my dir. Pushed in 749786.

        Show
        Kevin Clark added a comment - Oops, sorry about that. Was sitting around un-added in my dir. Pushed in 749786.
        Hide
        Esteve Fernandez added a comment -

        Yes, the TTwisted.py file was included in thrift-148_v9_20090302.patch but wasn't committed. It provides the server and client protocol classes for Twisted.

        Show
        Esteve Fernandez added a comment - Yes, the TTwisted.py file was included in thrift-148_v9_20090302.patch but wasn't committed. It provides the server and client protocol classes for Twisted.
        Hide
        Kevin Clark added a comment -

        Is v10 _lib something that got left out?

        Show
        Kevin Clark added a comment - Is v10 _lib something that got left out?
        Hide
        Esteve Fernandez added a comment -

        Thanks Kevin, but your commit lacks the required changes in the Python library. They were included in the original patch, but I've attached a simpler one which only updates the library.

        Show
        Esteve Fernandez added a comment - Thanks Kevin, but your commit lacks the required changes in the Python library. They were included in the original patch, but I've attached a simpler one which only updates the library.
        Hide
        Kevin Clark added a comment -

        In 749510. Thanks!

        Show
        Kevin Clark added a comment - In 749510. Thanks!
        Hide
        Esteve Fernandez added a comment -

        This version of the patch adds an IThriftClientFactory interface and a simple factory that implements it.

        Show
        Esteve Fernandez added a comment - This version of the patch adds an IThriftClientFactory interface and a simple factory that implements it.
        Hide
        Esteve Fernandez added a comment -

        I forgot to add the TCallbackTransport, here's an updated version that includes it.

        Show
        Esteve Fernandez added a comment - I forgot to add the TCallbackTransport, here's an updated version that includes it.
        Hide
        Esteve Fernandez added a comment -

        Changes the order in which Deferreds are created in the client.

        Show
        Esteve Fernandez added a comment - Changes the order in which Deferreds are created in the client.
        Hide
        Esteve Fernandez added a comment -

        Includes Dan's latest change (thanks!), uses the Int32StringReceiver protocol for framing and removed the TwistedMemoryBuffer transport.

        Show
        Esteve Fernandez added a comment - Includes Dan's latest change (thanks!), uses the Int32StringReceiver protocol for framing and removed the TwistedMemoryBuffer transport.
        Hide
        Terry Jones added a comment -

        I'm +1 on having this committed.

        I'm using this on a daily basis. I have 12 types of services sending 27 different Thrift structs back and forth with no problem. I have been tracking this patch and Esteve's updates to it over the last 4+ months. I've also spent time digging around in the generated Python, and it looks fine to me.

        This code is already in use by several others who are also tracking it as Thrift changes and Esteve releases new patches to track changes to Thrift trunk.

        Almost all of the changes in this patch are isolated from the rest of Thrift. This is because the patch adds an option to the compiler to generate Twisted-specific Python. The Python (ttypes.py) that is emitted for classes implementing Thrift structs is identical to the Python ordinarily produced. The compiler changes are isolated in an if clause that tests whether Twisted output has been requested.

        It would be great to get this committed. It only affects the people using Twisted, and while there may be future modifications to this functionality, it would be better and considerably less work for all concerned if this patch were in trunk. We can and will enhance the functionality, but it's much easier to do so in the context of a trunk which already has the underlying functionality provided by this patch.

        Because the basic functionality is already known to work (see above), patching one's copy to include future minor changes is likely optional to people already trying to work with Thrift and Twisted. Instead we can get on with other things and pick up future enhancements in a later Thrift release.

        Disclaimer: I work with Esteve - though he does the Thrift stuff entirely independent of things I work on. I just use the product of this patch.

        Terry

        Show
        Terry Jones added a comment - I'm +1 on having this committed. I'm using this on a daily basis. I have 12 types of services sending 27 different Thrift structs back and forth with no problem. I have been tracking this patch and Esteve's updates to it over the last 4+ months. I've also spent time digging around in the generated Python, and it looks fine to me. This code is already in use by several others who are also tracking it as Thrift changes and Esteve releases new patches to track changes to Thrift trunk. Almost all of the changes in this patch are isolated from the rest of Thrift. This is because the patch adds an option to the compiler to generate Twisted-specific Python. The Python (ttypes.py) that is emitted for classes implementing Thrift structs is identical to the Python ordinarily produced. The compiler changes are isolated in an if clause that tests whether Twisted output has been requested. It would be great to get this committed. It only affects the people using Twisted, and while there may be future modifications to this functionality, it would be better and considerably less work for all concerned if this patch were in trunk. We can and will enhance the functionality, but it's much easier to do so in the context of a trunk which already has the underlying functionality provided by this patch. Because the basic functionality is already known to work (see above), patching one's copy to include future minor changes is likely optional to people already trying to work with Thrift and Twisted. Instead we can get on with other things and pick up future enhancements in a later Thrift release. Disclaimer: I work with Esteve - though he does the Thrift stuff entirely independent of things I work on. I just use the product of this patch. Terry
        Hide
        Dan Di Spaltro added a comment -

        Esteve,
        I modified one line that basically changed the ways exceptions were being called in the thrift error callbacks. Before it was really hard to debug if you didn't know what you were doing, now it seems to generate a more proper traceback.

        Show
        Dan Di Spaltro added a comment - Esteve, I modified one line that basically changed the ways exceptions were being called in the thrift error callbacks. Before it was really hard to debug if you didn't know what you were doing, now it seems to generate a more proper traceback.
        Hide
        Esteve Fernandez added a comment -

        Returns the actual Deferred in process() for async methods, instead of defer.succeed(None)

        Show
        Esteve Fernandez added a comment - Returns the actual Deferred in process() for async methods, instead of defer.succeed(None)
        Hide
        Esteve Fernandez added a comment -

        Updated client and server scripts to use named arguments (see THRIFT-242) and interfaces.

        Show
        Esteve Fernandez added a comment - Updated client and server scripts to use named arguments (see THRIFT-242 ) and interfaces.
        Hide
        Esteve Fernandez added a comment -

        Andy, are you using TFramedTransport in your clients? You have to use it to wrap your transports, it's the same case as in TNonblockingServer. If you want to test, for example, PythonClient.py (included in the standard Thrift distribution), simply replace the line that reads:

        transport = TTransport.TBufferedTransport(transport)

        with:

        transport = TTransport.TFramedTransport(transport)

        In any case, I'll upload an updated version, which will report if you try to connect using a non-framed transport.

        Show
        Esteve Fernandez added a comment - Andy, are you using TFramedTransport in your clients? You have to use it to wrap your transports, it's the same case as in TNonblockingServer. If you want to test, for example, PythonClient.py (included in the standard Thrift distribution), simply replace the line that reads: transport = TTransport.TBufferedTransport(transport) with: transport = TTransport.TFramedTransport(transport) In any case, I'll upload an updated version, which will report if you try to connect using a non-framed transport.
        Hide
        Andy smith added a comment -

        Hmm... I've just applied the patch. Looks like I can get the python-python example working ( i.e. client.py can communicate with server.py) however trying to get client.py to talk to any of the other server implementations, or trying to use any of the other client implementations to talk to server.py doesn't work?

        Using svn checkout of thrift + current ( 8.2 twisted ) download

        Show
        Andy smith added a comment - Hmm... I've just applied the patch. Looks like I can get the python-python example working ( i.e. client.py can communicate with server.py) however trying to get client.py to talk to any of the other server implementations, or trying to use any of the other client implementations to talk to server.py doesn't work? Using svn checkout of thrift + current ( 8.2 twisted ) download
        Hide
        Esteve Fernandez added a comment -

        Dan: I've removed them, since handlers for other languages already implement Service.Iface

        Show
        Esteve Fernandez added a comment - Dan: I've removed them, since handlers for other languages already implement Service.Iface
        Hide
        Esteve Fernandez added a comment -

        Updated as of 20080119. Includes fixes by Dan Spaltro (thanks!) and drops IThriftService and handler interfaces.

        Show
        Esteve Fernandez added a comment - Updated as of 20080119. Includes fixes by Dan Spaltro (thanks!) and drops IThriftService and handler interfaces.
        Hide
        Dan Di Spaltro added a comment -

        I am not sure to be honest, It was fine just implementing Service.Iface, but I could see how the separation of the two makes sense.

        Show
        Dan Di Spaltro added a comment - I am not sure to be honest, It was fine just implementing Service.Iface, but I could see how the separation of the two makes sense.
        Hide
        Esteve Fernandez added a comment -

        Thanks for the clarification. Now I see where's the problem: the child handler interface doesn't inherit from the parent handler, but only from the service interface.

        Anyway, I don't know if the handler interface is a good idea or not (there's already the service interface). What do you think? What do you prefer to implement in handlers, Service.Iface or Service.IHandler?

        Show
        Esteve Fernandez added a comment - Thanks for the clarification. Now I see where's the problem: the child handler interface doesn't inherit from the parent handler, but only from the service interface. Anyway, I don't know if the handler interface is a good idea or not (there's already the service interface). What do you think? What do you prefer to implement in handlers, Service.Iface or Service.IHandler?
        Hide
        Dan Di Spaltro added a comment -

        Sure, I have one service (BaseService) which I extend with another class (SpecificService). Next thing I do is implement the handler for SpecificService and add the line zope.interface.implement(SpecificService) to the class. Zope complains about not being able to convert a class to the expected type, although I have no real zope experience I think it is expecting a different class, which in this case happens to be the BaseService. So the handler runs fine if I inherit from BaseService.IHandler or add the line zope.interface.implements(BaseService.Iface). Like I said I am not very familiar with this stuff, but am trying to use it. I can scrap together an example if I am not being clear enough

        Show
        Dan Di Spaltro added a comment - Sure, I have one service (BaseService) which I extend with another class (SpecificService). Next thing I do is implement the handler for SpecificService and add the line zope.interface.implement(SpecificService) to the class. Zope complains about not being able to convert a class to the expected type, although I have no real zope experience I think it is expecting a different class, which in this case happens to be the BaseService. So the handler runs fine if I inherit from BaseService.IHandler or add the line zope.interface.implements(BaseService.Iface). Like I said I am not very familiar with this stuff, but am trying to use it. I can scrap together an example if I am not being clear enough
        Hide
        Esteve Fernandez added a comment -

        Ouch, thanks for spotting this. I'll upload a newer version of the patch sometime soon, with our latest updates. However, I don't understand what you mean with "I had to use the base class Iface so zope wouldn't barf about the types not being correct". Could you please elaborate?

        Show
        Esteve Fernandez added a comment - Ouch, thanks for spotting this. I'll upload a newer version of the patch sometime soon, with our latest updates. However, I don't understand what you mean with "I had to use the base class Iface so zope wouldn't barf about the types not being correct". Could you please elaborate?
        Hide
        Dan Di Spaltro added a comment -

        It looks like something isn't working correctly when I apply that against trunk and generate a service that is extending another service it basically generates an incorrect header for the the Client class. It omits the parentheses and adds a comma. Even fixing it there is some more wierd activity, I had to use the base class Iface so zope wouldn't barf about the types not being correct. I which is applied directly against the tree + your changes + mine. I am not too familiar with Twisted, but see how this looks...

        Show
        Dan Di Spaltro added a comment - It looks like something isn't working correctly when I apply that against trunk and generate a service that is extending another service it basically generates an incorrect header for the the Client class. It omits the parentheses and adds a comma. Even fixing it there is some more wierd activity, I had to use the base class Iface so zope wouldn't barf about the types not being correct. I which is applied directly against the tree + your changes + mine. I am not too familiar with Twisted, but see how this looks...
        Hide
        Esteve Fernandez added a comment -

        Updated patch (as of 20081230). Includes support for Zope/Twisted interfaces.

        Show
        Esteve Fernandez added a comment - Updated patch (as of 20081230). Includes support for Zope/Twisted interfaces.
        Hide
        Esteve Fernandez added a comment -

        Here's an updated patch which works with newer Thrift versions (as of 20081115)

        Show
        Esteve Fernandez added a comment - Here's an updated patch which works with newer Thrift versions (as of 20081115)
        Hide
        Esteve Fernandez added a comment -

        Forgot to add Twisted related classes (protocols, factories, etc.)

        Show
        Esteve Fernandez added a comment - Forgot to add Twisted related classes (protocols, factories, etc.)
        Hide
        Esteve Fernandez added a comment -

        I don't know about Ruby, but in Python, a TMemoryBuffer is just a wrapper around StringIO (which is pretty fast in its C version). My main goal was to make it feel as natural as possible to both communities (Twisted and Thrift)

        Show
        Esteve Fernandez added a comment - I don't know about Ruby, but in Python, a TMemoryBuffer is just a wrapper around StringIO (which is pretty fast in its C version). My main goal was to make it feel as natural as possible to both communities (Twisted and Thrift)
        Hide
        Ben Taitelbaum added a comment -

        So from looking at your patch, it looks like to avoid having blocking reads when data isn't available, you read it all into a memory buffer, and then perform normal processing on that buffer, relying on an EOF exception if there isn't enough data available.

        I think this solution will work in EventMachine as well, although I was hoping to do something more efficient, like be able to tell if there was enough data for a particular message more quickly (like reading bytes out of a buffer, and backing up as soon as you can tell that not enough bytes are available), but even with binaryprotocol, this was not easy, and with other protocols, it would be even harder.

        Have you though about optimizations to the way you're doing network handling, or in practice is the overhead of creating a TMemoryBuffer and going until an EOF not really a bottleneck?

        Show
        Ben Taitelbaum added a comment - So from looking at your patch, it looks like to avoid having blocking reads when data isn't available, you read it all into a memory buffer, and then perform normal processing on that buffer, relying on an EOF exception if there isn't enough data available. I think this solution will work in EventMachine as well, although I was hoping to do something more efficient, like be able to tell if there was enough data for a particular message more quickly (like reading bytes out of a buffer, and backing up as soon as you can tell that not enough bytes are available), but even with binaryprotocol, this was not easy, and with other protocols, it would be even harder. Have you though about optimizations to the way you're doing network handling, or in practice is the overhead of creating a TMemoryBuffer and going until an EOF not really a bottleneck?
        Hide
        Esteve Fernandez added a comment -

        I'm not sure if EventMachine is the exact Ruby equivalent of Twisted, but both implement the Reactor pattern and support Futures (Deferreds in Twisted-speak, Deferrables in EventMachine)

        Here's what I did in the client code:

        1 - maintain a dictionary of Deferreds in your Client, keyed on seqid
        2 - whenever a method needs to return a value, return a Deferred instead
        3 - read incoming messages in the dataReceived method (client protocol)
        4 - when the consumeFrame method encounters a complete message, call the appropiate recv_METHOD method in the client
        5 - once the recv_METHOD method finishes, fire the Deferred

        And this in the server code to support methods that return Deferreds:

        1 - read incoming messages in the dataReceived method (server protocol)
        2 - when the consumeFrame method encounters a complete message, call the Processor's process method
        3 - wrap the self._handler.METHOD in a Deferred (with maybeDeferred)
        4 - add a callback (and an errback if the method is supposed to throw an exception) to the previous Deferred

        Show
        Esteve Fernandez added a comment - I'm not sure if EventMachine is the exact Ruby equivalent of Twisted, but both implement the Reactor pattern and support Futures ( Deferreds in Twisted-speak, Deferrables in EventMachine) Here's what I did in the client code: 1 - maintain a dictionary of Deferreds in your Client, keyed on seqid 2 - whenever a method needs to return a value, return a Deferred instead 3 - read incoming messages in the dataReceived method (client protocol) 4 - when the consumeFrame method encounters a complete message, call the appropiate recv_METHOD method in the client 5 - once the recv_METHOD method finishes, fire the Deferred And this in the server code to support methods that return Deferreds: 1 - read incoming messages in the dataReceived method (server protocol) 2 - when the consumeFrame method encounters a complete message, call the Processor's process method 3 - wrap the self._handler.METHOD in a Deferred (with maybeDeferred) 4 - add a callback (and an errback if the method is supposed to throw an exception) to the previous Deferred
        Hide
        Ben Taitelbaum added a comment -

        Do you know if Twisted is comparable to EventMachine for ruby? I'm curious if you have any thoughts on whether or not the approach you've taken would be suitable for THRIFT-146?

        Show
        Ben Taitelbaum added a comment - Do you know if Twisted is comparable to EventMachine for ruby? I'm curious if you have any thoughts on whether or not the approach you've taken would be suitable for THRIFT-146 ?
        Hide
        Esteve Fernandez added a comment -

        Server for the Twisted enabled Calculator example.

        Show
        Esteve Fernandez added a comment - Server for the Twisted enabled Calculator example.
        Hide
        Esteve Fernandez added a comment -

        Client for the Twisted enabled Calculator example.

        Show
        Esteve Fernandez added a comment - Client for the Twisted enabled Calculator example.
        Hide
        Esteve Fernandez added a comment -

        Adds support for Twisted in the Thrift compiler and the Python libraries.

        Show
        Esteve Fernandez added a comment - Adds support for Twisted in the Thrift compiler and the Python libraries.

          People

          • Assignee:
            Esteve Fernandez
            Reporter:
            Esteve Fernandez
          • Votes:
            6 Vote for this issue
            Watchers:
            6 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development