Thrift
  1. Thrift
  2. THRIFT-1353

Switch to performance branch, get rid of BinaryParser

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 0.7
    • Fix Version/s: 0.9.1
    • Component/s: Node.js - Library
    • Labels:

      Description

      I vote that the thrift library switches to the performance branch at https://github.com/wadey/node-thrift/tree/performance

      I much prefer using the node.js buffers over the BinaryParser class, the performance branch seems focused on this very issue. Also it includes the excellent int64 implementation from broofa. What's not to like?

        Issue Links

          Activity

          Hide
          Roger Meier added a comment -

          Hi Hans!

          Thanks for bringing new ideas and bugfixes to Thrift Node.js implementation!

          Could you please create a patch and grant License to ASF?
          see: http://wiki.apache.org/thrift/HowToContribute

          Another important topic are Unit Tests, see my comments on THRIFT-1261
          do you have some?

          I'm looking forward to commit patches for Node.js!

          Thanks
          roger

          Show
          Roger Meier added a comment - Hi Hans! Thanks for bringing new ideas and bugfixes to Thrift Node.js implementation! Could you please create a patch and grant License to ASF? see: http://wiki.apache.org/thrift/HowToContribute Another important topic are Unit Tests, see my comments on THRIFT-1261 do you have some? I'm looking forward to commit patches for Node.js! Thanks roger
          Hide
          Wade Simmons added a comment -

          I'm the author of this upstream branch (and the original author of the Node.js support). I plan to spend some time this weekend working on getting a patch made from this branch so we can get it merged upstream. There are a few things that aren't backwards compatible, so we will need to make sure that is properly described in the 0.8 release.

          Show
          Wade Simmons added a comment - I'm the author of this upstream branch (and the original author of the Node.js support). I plan to spend some time this weekend working on getting a patch made from this branch so we can get it merged upstream. There are a few things that aren't backwards compatible, so we will need to make sure that is properly described in the 0.8 release.
          Hide
          Wade Simmons added a comment -

          I merged node-thrift 0.7.0 with my performance branch, and plan to submit to Thrift for 0.8.0. Can I get a few people to test this?

          You will need this compiler patch:

          https://issues.apache.org/jira/browse/THRIFT-1351

          Two non-backwords compatible changes:

          Show
          Wade Simmons added a comment - I merged node-thrift 0.7.0 with my performance branch, and plan to submit to Thrift for 0.8.0. Can I get a few people to test this? `thrift-dev` on npm https://github.com/wadey/node-thrift/tree/performance You will need this compiler patch: https://issues.apache.org/jira/browse/THRIFT-1351 Two non-backwords compatible changes: "binary" fields will be returned as Buffers instead of Strings "int64" fields will be returned as http://github.com/broofa/node-int64 objects
          Hide
          Wade Simmons added a comment -

          All of the feedback I've heard is positive. Can we get this merged in?

          Show
          Wade Simmons added a comment - All of the feedback I've heard is positive. Can we get this merged in?
          Hide
          Roger Meier added a comment -

          Your repo does not reflect the complete Thrift codebase, e.g. compiler is missing. I recommend you to fork from the Apache Thrift github mirror located at https://github.com/apache/thrift. This will simplify code sync and enable better alignment of the two JavaScript implementations and nodejs imrovements... we did also a lot of changes on the compiler! the other thing is the testsuite, see THRIFT-1134

          It is really difficult to manage a project with so many languages and that's why unit test and cross language tests are so important, see THRIFT-847 => test/test.sh integrates also some node cross language tests

          I really like to merge your stuff in, but I need a patch and whenever possible unit tests.

          Show
          Roger Meier added a comment - Your repo does not reflect the complete Thrift codebase, e.g. compiler is missing. I recommend you to fork from the Apache Thrift github mirror located at https://github.com/apache/thrift . This will simplify code sync and enable better alignment of the two JavaScript implementations and nodejs imrovements... we did also a lot of changes on the compiler! the other thing is the testsuite, see THRIFT-1134 It is really difficult to manage a project with so many languages and that's why unit test and cross language tests are so important, see THRIFT-847 => test/test.sh integrates also some node cross language tests I really like to merge your stuff in, but I need a patch and whenever possible unit tests.
          Hide
          Henrique Mendonça added a comment -

          I had a look on Wade's changes ages ago and they seemed to be very nicely done. Since he seems to be no longer reachable I'm submitting the re-based patch in his name. Plus some minor changes on the tests.
          THRIFT-1353-noBinaryParser-BufferedTrans-Int64.patch

          This fixes the problems with Int64 as well as improves the buffered transport impl. and also remove the use of the slow binary parser as he says above.

          There is still a lot to do on the nodejs library but this is a great addiction. Could anyone test/commit this?

          By the way, does anyone knows how we can install node on the jenkins slave?

          Cheers!

          Show
          Henrique Mendonça added a comment - I had a look on Wade's changes ages ago and they seemed to be very nicely done. Since he seems to be no longer reachable I'm submitting the re-based patch in his name. Plus some minor changes on the tests. THRIFT-1353 -noBinaryParser-BufferedTrans-Int64.patch This fixes the problems with Int64 as well as improves the buffered transport impl. and also remove the use of the slow binary parser as he says above. There is still a lot to do on the nodejs library but this is a great addiction. Could anyone test/commit this? By the way, does anyone knows how we can install node on the jenkins slave? Cheers!
          Hide
          Jake Farrell added a comment -

          Henrique, ill take a look at this patch later tonight. Roger or I can see about getting node tests setup within jenkins

          Show
          Jake Farrell added a comment - Henrique, ill take a look at this patch later tonight. Roger or I can see about getting node tests setup within jenkins
          Hide
          Wade Simmons added a comment -

          Henrique Mendonca: Thanks for pushing forward on getting this merged in. I am no longer using Node.js and Thrift at work, so I have a hard time finding time to give to this effort. I'll step back and let those that are actively using this make the decisions, although you can reach out to me if there any specific questions about my original code that needs answering.

          Show
          Wade Simmons added a comment - Henrique Mendonca: Thanks for pushing forward on getting this merged in. I am no longer using Node.js and Thrift at work, so I have a hard time finding time to give to this effort. I'll step back and let those that are actively using this make the decisions, although you can reach out to me if there any specific questions about my original code that needs answering.
          Hide
          Henrique Mendonça added a comment -

          Thanks for you feedback Wade. I think we should check this in as it fix some bugs and improve other stuff. Got your code to run but didn't manage to create a proper patch (too many files and other conflicts). I also don't work directly with it, only had some ideas which would require it, so it's also hard for me to dedicate time for it. However, I should get this running within the next weeks.
          Thanks for your contribution anyway!
          Henrique

          Show
          Henrique Mendonça added a comment - Thanks for you feedback Wade. I think we should check this in as it fix some bugs and improve other stuff. Got your code to run but didn't manage to create a proper patch (too many files and other conflicts). I also don't work directly with it, only had some ideas which would require it, so it's also hard for me to dedicate time for it. However, I should get this running within the next weeks. Thanks for your contribution anyway! Henrique
          Hide
          Henrique Mendonça added a comment - - edited

          Here it is, finally.
          THRIFT-1353-noBinaryParser-BufferedTrans-Int64-v3.patch
          cleaned up and rebased for today's trunk
          It also includes the files from node-int64

          ps: the test from cpp client to nodejs server still fails but we're getting there...

          ps2: rewriting the nodejs test client to use the assert module would be also nice...

          ps3: patch with -p0 please

          Show
          Henrique Mendonça added a comment - - edited Here it is, finally. THRIFT-1353 -noBinaryParser-BufferedTrans-Int64-v3.patch cleaned up and rebased for today's trunk It also includes the files from node-int64 ps: the test from cpp client to nodejs server still fails but we're getting there... ps2: rewriting the nodejs test client to use the assert module would be also nice... ps3: patch with -p0 please
          Hide
          Tom Jack added a comment -

          Is there a reason to both include the files from node-int64 and to depend on it in package.json?

          Show
          Tom Jack added a comment - Is there a reason to both include the files from node-int64 and to depend on it in package.json?
          Hide
          Henrique Mendonça added a comment -

          Not really, just wanted to facilitate it for the committers and testers. Talked briefly to Roger before but I wasn't sure whether Jenkins would handle the NPM. We can definitely leave the files from node-int64 uncommitted if it works.

          Thanks for having a look anyways!

          Show
          Henrique Mendonça added a comment - Not really, just wanted to facilitate it for the committers and testers. Talked briefly to Roger before but I wasn't sure whether Jenkins would handle the NPM. We can definitely leave the files from node-int64 uncommitted if it works. Thanks for having a look anyways!
          Hide
          Henrique Mendonça added a comment -

          finally committed!

          Show
          Henrique Mendonça added a comment - finally committed!
          Hide
          Hudson added a comment -

          Integrated in Thrift #571 (See https://builds.apache.org/job/Thrift/571/)
          Thrift-1353: Switch to performance branch, get rid of BinaryParser
          Client: Node.js
          Patch: Wade Simmons

          "binary" fields will be returned as Buffers instead of Strings
          "int64" fields will be returned as http://github.com/broofa/node-int64 objects (Revision 1401081)

          Result = ABORTED

          Show
          Hudson added a comment - Integrated in Thrift #571 (See https://builds.apache.org/job/Thrift/571/ ) Thrift-1353: Switch to performance branch, get rid of BinaryParser Client: Node.js Patch: Wade Simmons "binary" fields will be returned as Buffers instead of Strings "int64" fields will be returned as http://github.com/broofa/node-int64 objects (Revision 1401081) Result = ABORTED
          Hide
          John Eckhart added a comment -

          Looks like you missed a file (binary.js) which is not the same as binary_parser.js that is in the release branch. Here's a link to the original file: https://raw.github.com/wadey/node-thrift/performance/lib/thrift/binary.js

          Show
          John Eckhart added a comment - Looks like you missed a file (binary.js) which is not the same as binary_parser.js that is in the release branch. Here's a link to the original file: https://raw.github.com/wadey/node-thrift/performance/lib/thrift/binary.js
          Hide
          Jake Farrell added a comment -

          John, We can not copy files from github and just add them to the codebase due to legal rights, please see http://thrift.apache.org/docs/HowToContribute/ for details on making contributions

          Show
          Jake Farrell added a comment - John, We can not copy files from github and just add them to the codebase due to legal rights, please see http://thrift.apache.org/docs/HowToContribute/ for details on making contributions
          Hide
          John Eckhart added a comment -

          I appreciate that you can't copy that file without following your process; however, I am pointing out that your most recent merge missed a file and therefore broke the node client. The missing file is also in the patch that the original author attached to the ticket after following the contribution process.

          Show
          John Eckhart added a comment - I appreciate that you can't copy that file without following your process; however, I am pointing out that your most recent merge missed a file and therefore broke the node client. The missing file is also in the patch that the original author attached to the ticket after following the contribution process.
          Hide
          Jake Farrell added a comment -

          Sorry John, didnt understand what your comment was implying. Henrique can you please take a look at this as you where the committer

          Show
          Jake Farrell added a comment - Sorry John, didnt understand what your comment was implying. Henrique can you please take a look at this as you where the committer
          Hide
          Hudson added a comment -

          Integrated in Thrift #576 (See https://builds.apache.org/job/Thrift/576/)
          Thrift-1353: Switch to performance branch, get rid of BinaryParser
          Client: Node.js
          Patch: Wade Simmons

          add missing files and licenses (Revision 1402414)

          Result = ABORTED

          Show
          Hudson added a comment - Integrated in Thrift #576 (See https://builds.apache.org/job/Thrift/576/ ) Thrift-1353: Switch to performance branch, get rid of BinaryParser Client: Node.js Patch: Wade Simmons add missing files and licenses (Revision 1402414) Result = ABORTED
          Hide
          John Eckhart added a comment -

          Looks good now, thanks!

          Show
          John Eckhart added a comment - Looks good now, thanks!
          Hide
          Wade Simmons added a comment -

          Just want to make a clarification for people reading this ticket: This patch has not been merged on to the 0.9.x branch yet even though the "fix version" says 0.9.1. It is only on master currently.

          Show
          Wade Simmons added a comment - Just want to make a clarification for people reading this ticket: This patch has not been merged on to the 0.9.x branch yet even though the "fix version" says 0.9.1. It is only on master currently.

            People

            • Assignee:
              Henrique Mendonça
              Reporter:
              Hans Duedal
            • Votes:
              2 Vote for this issue
              Watchers:
              8 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development