Details

    • Type: Sub-task Sub-task
    • Status: Open
    • Priority: Major Major
    • Resolution: Unresolved
    • Affects Version/s: None
    • Fix Version/s: None
    • Labels:
      None

      Description

      I'm raising this JIRA on behalf of Erik Bernhardsson.
      Seems this was submitted to the JIRA list on Wed 9 Sep 2009, but I didn't see it until I skimmed the JIRA list feed for thrift-dev.

      I'll attach his original files shortly, though I may just fold them into an SVN trunk checkout first and post the diff.

      1. CppClient.cpp
        3 kB
        Bruce Simpson
      2. CppServer.cpp
        5 kB
        Bruce Simpson
      3. erikb-asio.patch
        81 kB
        Bruce Simpson
      4. TAsioAsync.cpp
        10 kB
        Bruce Simpson
      5. TAsioAsync.h
        5 kB
        Bruce Simpson
      6. TAsync.h
        1 kB
        Bruce Simpson
      7. TFuture.h
        11 kB
        Bruce Simpson
      8. THRIFT-579_against_trunk_1243124.patch
        88 kB
        Christian Taedcke
      9. THRIFT-579_v2_against_trunk_1304085.patch
        90 kB
        Christian Taedcke
      10. thrift-async-817938.diff
        87 kB
        Bruce Simpson

        Issue Links

          Activity

          Hide
          Don Scott added a comment -

          It looks like this none of the issues Ben pointed out have been touched. Would anyone like to take ownership of these fixes?

          Show
          Don Scott added a comment - It looks like this none of the issues Ben pointed out have been touched. Would anyone like to take ownership of these fixes?
          Hide
          Ben Craig added a comment -

          I have made a first stab at the code review. After some of those issues are resolved, I will look at the remaining parts of the code.

          Show
          Ben Craig added a comment - I have made a first stab at the code review. After some of those issues are resolved, I will look at the remaining parts of the code.
          Hide
          Ben Craig added a comment -

          I have set up a review at https://reviews.apache.org/r/14984/

          I have not actually done the review yet, but one step at a time

          I have skimmed the code, and there will be some things that I will want changed before submitting.

          Show
          Ben Craig added a comment - I have set up a review at https://reviews.apache.org/r/14984/ I have not actually done the review yet, but one step at a time I have skimmed the code, and there will be some things that I will want changed before submitting.
          Hide
          Ben Craig added a comment -

          I will look at this for this release. It will probably be a few weeks before I can devote any time to looking at the patch in earnest.

          Show
          Ben Craig added a comment - I will look at this for this release. It will probably be a few weeks before I can devote any time to looking at the patch in earnest.
          Hide
          Tobias Markmann added a comment -

          What's the status of this? I'd really like to see support for boost asio, mainly to reduce the number of requirements for thrift c++ and thereby ease porting to other platforms. Thrift already requires boost and by using asio one could have async client/server without libevent.

          Show
          Tobias Markmann added a comment - What's the status of this? I'd really like to see support for boost asio, mainly to reduce the number of requirements for thrift c++ and thereby ease porting to other platforms. Thrift already requires boost and by using asio one could have async client/server without libevent.
          Hide
          Roger Meier added a comment -

          Please rebase, add a test case and you are in!

          Show
          Roger Meier added a comment - Please rebase, add a test case and you are in!
          Hide
          Christian Taedcke added a comment -

          Hi Sergei,
          the code is at my repository at bitbucket in the branch named THRIFT-579.
          I would be happy if someone would continue the work.

          Show
          Christian Taedcke added a comment - Hi Sergei, the code is at my repository at bitbucket in the branch named THRIFT-579 . I would be happy if someone would continue the work.
          Hide
          Sergei Petrunia added a comment -

          Nope, they seem to be different. THRIFT-579_v2_against_trunk_1304085.patch uses Boost ASIO, https://bitbucket.org/chrta/thrift/overview uses libevent, etc. It would be interesting to know how they compare.

          Show
          Sergei Petrunia added a comment - Nope, they seem to be different. THRIFT-579 _v2_against_trunk_1304085.patch uses Boost ASIO, https://bitbucket.org/chrta/thrift/overview uses libevent, etc. It would be interesting to know how they compare.
          Hide
          Sergei Petrunia added a comment -

          Could anybody tell, are THRIFT-579_v2_against_trunk_1304085.patch and https://bitbucket.org/chrta/thrift/overview the same, or they are two different (alternative?) implementations?

          Show
          Sergei Petrunia added a comment - Could anybody tell, are THRIFT-579 _v2_against_trunk_1304085.patch and https://bitbucket.org/chrta/thrift/overview the same, or they are two different (alternative?) implementations?
          Hide
          Sergei Petrunia added a comment -

          Hello,

          +1 vote for getting this into trunk!

          I've tried using THRIFT-579_v2_against_trunk_1304085.patch, and after I applied the suggestion by Michiel Ephraim, it compiled. My async client worked for me so far, except for one problem: the library doesn't notify the client when connection could not be established.

          This is caused by a missing part in the code, check out lib/cpp/src/async/TAsioAsync.cpp, function
          TAsioClient::handleConnect(). It has comment that says "Todo: call user-provided errback".

          Show
          Sergei Petrunia added a comment - Hello, +1 vote for getting this into trunk! I've tried using THRIFT-579 _v2_against_trunk_1304085.patch, and after I applied the suggestion by Michiel Ephraim, it compiled. My async client worked for me so far, except for one problem: the library doesn't notify the client when connection could not be established. This is caused by a missing part in the code, check out lib/cpp/src/async/TAsioAsync.cpp, function TAsioClient::handleConnect(). It has comment that says "Todo: call user-provided errback".
          Hide
          Henrique Mendonça added a comment -

          +1 that!

          Show
          Henrique Mendonça added a comment - +1 that!
          Hide
          Roger Meier added a comment -

          a test case and I commit
          ;-r

          Show
          Roger Meier added a comment - a test case and I commit ;-r
          Hide
          Christian Taedcke added a comment -

          I adapted my code at https://bitbucket.org/chrta/thrift/overview and rebased it to the current svn head revision. As soon as i get some feedback, i can upload a new patch.

          Are there any other issues with the patch?
          What is missing to get this committed?

          Show
          Christian Taedcke added a comment - I adapted my code at https://bitbucket.org/chrta/thrift/overview and rebased it to the current svn head revision. As soon as i get some feedback, i can upload a new patch. Are there any other issues with the patch? What is missing to get this committed?
          Hide
          Michiel Ephraim added a comment -

          For the next version of the patch, please add "src/async/TAsync.h" and "src/async/TFuture.h" to the "include_async_HEADERS" variable in the file "thrift/lib/cpp/Makefile.am". This change causes these include-files to be installed during "make install".

          Show
          Michiel Ephraim added a comment - For the next version of the patch, please add "src/async/TAsync.h" and "src/async/TFuture.h" to the "include_async_HEADERS" variable in the file "thrift/lib/cpp/Makefile.am". This change causes these include-files to be installed during "make install".
          Hide
          Christian Taedcke added a comment - - edited

          I rebased the patch against current trunk. Additionally i fixed some issues with Visual Studio 2010.

          Show
          Christian Taedcke added a comment - - edited I rebased the patch against current trunk. Additionally i fixed some issues with Visual Studio 2010.
          Hide
          Michiel Ephraim added a comment -

          Is there any way I can help this issue move forward? I would love to see these patches in the next release.

          Show
          Michiel Ephraim added a comment - Is there any way I can help this issue move forward? I would love to see these patches in the next release.
          Hide
          Alexander Verbitskiy added a comment -

          Christian Taedcke, I'm working on it.

          Show
          Alexander Verbitskiy added a comment - Christian Taedcke, I'm working on it.
          Hide
          Christian Taedcke added a comment -

          I adapted the previous patch thrift-async-817938.diff to the current trunk.
          The tutorial is working with this code.

          Show
          Christian Taedcke added a comment - I adapted the previous patch thrift-async-817938.diff to the current trunk. The tutorial is working with this code.
          Hide
          Christian Taedcke added a comment -

          I adapted the patch to the current trunk, but the generated code does not compile yet. I would be glad if someone could help me with it to prepare a new patch:

          Branch THRIFT-579 at https://bitbucket.org/chrta/thrift/overview

          Show
          Christian Taedcke added a comment - I adapted the patch to the current trunk, but the generated code does not compile yet. I would be glad if someone could help me with it to prepare a new patch: Branch THRIFT-579 at https://bitbucket.org/chrta/thrift/overview
          Hide
          Jake Farrell added a comment -

          The code from this patch would need to be rebased against the current trunk, test cases added as well as some documentation and any licensing cleanup and mentioned above. http://wiki.apache.org/thrift/HowToContribute

          Show
          Jake Farrell added a comment - The code from this patch would need to be rebased against the current trunk, test cases added as well as some documentation and any licensing cleanup and mentioned above. http://wiki.apache.org/thrift/HowToContribute
          Hide
          Andrey Karmazin added a comment -

          UP

          Show
          Andrey Karmazin added a comment - UP
          Hide
          Valery Kreidenko added a comment -

          Hello,

          would be interesting to know if any progress on this JIRA and any plans to incorporate this important feature into nearest releases.

          Thanx in advance.
          Valery.

          Show
          Valery Kreidenko added a comment - Hello, would be interesting to know if any progress on this JIRA and any plans to incorporate this important feature into nearest releases. Thanx in advance. Valery.
          Hide
          Bruce Simpson added a comment -

          Updated patch containing Erik and Mattias's code at svn rev 817938.
          Compilation now verified on FreeBSD 7.2-STABLE with Boost 1.39.
          ASF license prepended, whitespace/style not fixed.
          GlobalOutput() substituted for std::cout.

          Show
          Bruce Simpson added a comment - Updated patch containing Erik and Mattias's code at svn rev 817938. Compilation now verified on FreeBSD 7.2-STABLE with Boost 1.39. ASF license prepended, whitespace/style not fixed. GlobalOutput() substituted for std::cout.
          Hide
          Erik Bernhardsson added a comment -

          Everything we posted is intended to be under ASF.

          I'm not a regular open source contributor, but I realize I obviously should
          have added the header. Let me know if I need to clarify anything else.

          And as pointed out, Mattias de Zalenski should also be credited as much as
          me for the code we submitted.

          Erik Bernhardsson
          Spotify AB

          Show
          Erik Bernhardsson added a comment - Everything we posted is intended to be under ASF. I'm not a regular open source contributor, but I realize I obviously should have added the header. Let me know if I need to clarify anything else. And as pointed out, Mattias de Zalenski should also be credited as much as me for the code we submitted. Erik Bernhardsson Spotify AB
          Hide
          Bruce Simpson added a comment -

          Erik, Mattias: Can you clarify the licensing position of this code?
          Some of the files submitted don't have the ASF license in the header, so
          Esteve is quite right to raise a flag here about the submission.

          [Given that Mattias has already responded to the JIRA ticket, and that
          Erik had submitted the code to the JIRA list (although w/o raising a
          ticket), it seemed reasonable to infer that they intended to share the
          code.]

          P.S. Mattias, thanks for clarifying that you are co-author of the
          submission.

          thanks,
          BMS

          Show
          Bruce Simpson added a comment - Erik, Mattias: Can you clarify the licensing position of this code? Some of the files submitted don't have the ASF license in the header, so Esteve is quite right to raise a flag here about the submission. [Given that Mattias has already responded to the JIRA ticket, and that Erik had submitted the code to the JIRA list (although w/o raising a ticket), it seemed reasonable to infer that they intended to share the code.] P.S. Mattias, thanks for clarifying that you are co-author of the submission. thanks, BMS
          Hide
          Esteve Fernandez added a comment -

          IANAL, but I don't know if you can attach files on behalf of other people, Bruce. I don't know if Erik or Mattias have signed their CLAs, but in any case we should be very careful with IP.

          Show
          Esteve Fernandez added a comment - IANAL, but I don't know if you can attach files on behalf of other people, Bruce. I don't know if Erik or Mattias have signed their CLAs, but in any case we should be very careful with IP.
          Hide
          Mattias de Zalenski added a comment -

          Hi!

          Two notes:

          • Our main objective is a fully async client/server for C++. Only the files
            with asio in their name and the tutorial example depend on boost asio.
          • I am a collaborator as much as Erik.

          It is a bit unfortunate that the ASIO parts stand out so much. Perhaps we
          should add a small select()-based example, and split the async and asio
          changes into different JIRA:s?

          Thanks for picking up our code though!

          Regards,
          Mattias


          Mattias de Zalenski
          Software Developer
          Spotify

          Tel: +46 70 487 74 63
          Fax: +46 8 510 62 416

          This e-mail (including any attachments) may contain information that is
          confidential and/or privileged. It is intended only for the recipient(s).
          If you have reason to believe that you are not the intended recipient of
          this e-mail, please contact the sender immediately and delete the e-mail
          from your computer.

          Show
          Mattias de Zalenski added a comment - Hi! Two notes: Our main objective is a fully async client/server for C++. Only the files with asio in their name and the tutorial example depend on boost asio. I am a collaborator as much as Erik. It is a bit unfortunate that the ASIO parts stand out so much. Perhaps we should add a small select()-based example, and split the async and asio changes into different JIRA:s? Thanks for picking up our code though! Regards, Mattias – Mattias de Zalenski Software Developer Spotify Tel: +46 70 487 74 63 Fax: +46 8 510 62 416 This e-mail (including any attachments) may contain information that is confidential and/or privileged. It is intended only for the recipient(s). If you have reason to believe that you are not the intended recipient of this e-mail, please contact the sender immediately and delete the e-mail from your computer.
          Hide
          Bruce Simpson added a comment -

          Whitespace has not been fixed in diff to match Thrift style.

          Show
          Bruce Simpson added a comment - Whitespace has not been fixed in diff to match Thrift style.
          Hide
          Bruce Simpson added a comment -

          Erik Bernhardsson's original files

          Show
          Bruce Simpson added a comment - Erik Bernhardsson's original files
          Hide
          Bruce Simpson added a comment -

          Erik Bernhardsson's original files

          Show
          Bruce Simpson added a comment - Erik Bernhardsson's original files
          Hide
          Bruce Simpson added a comment -

          Erik Bernhardsson's code as a patch against SVN trunk @814262

          Show
          Bruce Simpson added a comment - Erik Bernhardsson's code as a patch against SVN trunk @814262

            People

            • Assignee:
              Ben Craig
              Reporter:
              Bruce Simpson
            • Votes:
              20 Vote for this issue
              Watchers:
              18 Start watching this issue

              Dates

              • Created:
                Updated:

                Development