Thrift
  1. Thrift
  2. THRIFT-1177

Update thrift to reflect changes in Go's networking libraries

    Details

    • Type: Dependency upgrade Dependency upgrade
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 0.7
    • Fix Version/s: 0.7
    • Component/s: Go - Compiler, Go - Library
    • Labels:
      None
    • Patch Info:
      Patch Available

      Description

      Go updated its networking libraries requiring a change to both the thrift code generator and the libraries.
      Additionally, the Go generator did not support arguments/variables that were Go keywords, this needs to be updated to work with Cassandra's thrift since "range" is a Go keyword.

      1. gopatch2-r2.diff
        22 kB
        Trevor Summers Smith
      2. gopatch2.diff
        22 kB
        Aalok Shah

        Activity

        Hide
        Hudson added a comment -

        Integrated in Thrift #191 (See https://builds.apache.org/job/Thrift/191/)
        THRIFT-1177. go: Update thrift to reflect changes in Go's networking libraries

        Patch: Aalok Shah

        bryanduxbury : http://svn.apache.org/viewvc/?view=rev&rev=1146167
        Files :

        • /thrift/trunk/compiler/cpp/src/generate/t_go_generator.cc
        • /thrift/trunk/lib/go/thrift/ttransport_test.go
        • /thrift/trunk/lib/go/thrift/ttransport.go
        • /thrift/trunk/lib/go/thrift/tprotocol_test.go
        • /thrift/trunk/lib/go/Make.deps
        • /thrift/trunk/lib/go/thrift/tsocket.go
        • /thrift/trunk/lib/go/thrift/tnonblocking_socket.go
        • /thrift/trunk/lib/go/thrift/thttp_client.go
        • /thrift/trunk/lib/go/thrift/thttp_client_test.go
        Show
        Hudson added a comment - Integrated in Thrift #191 (See https://builds.apache.org/job/Thrift/191/ ) THRIFT-1177 . go: Update thrift to reflect changes in Go's networking libraries Patch: Aalok Shah bryanduxbury : http://svn.apache.org/viewvc/?view=rev&rev=1146167 Files : /thrift/trunk/compiler/cpp/src/generate/t_go_generator.cc /thrift/trunk/lib/go/thrift/ttransport_test.go /thrift/trunk/lib/go/thrift/ttransport.go /thrift/trunk/lib/go/thrift/tprotocol_test.go /thrift/trunk/lib/go/Make.deps /thrift/trunk/lib/go/thrift/tsocket.go /thrift/trunk/lib/go/thrift/tnonblocking_socket.go /thrift/trunk/lib/go/thrift/thttp_client.go /thrift/trunk/lib/go/thrift/thttp_client_test.go
        Hide
        Bryan Duxbury added a comment -

        I just committed this. Thanks for the patch, Aalok!

        Show
        Bryan Duxbury added a comment - I just committed this. Thanks for the patch, Aalok!
        Hide
        Aalok Shah added a comment -

        The first patch (gopatch2.diff) works with the latest stable release (r58) of Go released on June 30.
        The second patch (gopatch2-r2.diff) works with the previous stable release (r57) of Go.

        Use the first patch.

        Show
        Aalok Shah added a comment - The first patch (gopatch2.diff) works with the latest stable release (r58) of Go released on June 30. The second patch (gopatch2-r2.diff) works with the previous stable release (r57) of Go. Use the first patch.
        Hide
        Bryan Duxbury added a comment -

        Just to be clear, it's the first patch that should be committed, correct? That's the one that works with stable Go interpreter?

        Show
        Bryan Duxbury added a comment - Just to be clear, it's the first patch that should be committed, correct? That's the one that works with stable Go interpreter?
        Hide
        Aalok Shah added a comment -

        It's been over a month since this code review was posted. Anyone interested in committing this or should I keep telling people to use my github repo instead?

        Show
        Aalok Shah added a comment - It's been over a month since this code review was posted. Anyone interested in committing this or should I keep telling people to use my github repo instead?
        Hide
        Aalok Shah added a comment -

        It appears in the current stable release (r57.1) of Go, http.Get() returns 3 values.
        In the latest development version, http.Get() returns 2 values.
        As there is no way to perform conditional compilations (a la #ifdef in C), a version can either work with the stable release or tip, but not both.

        I assume the Thrift project prefers working with the latest stable release rather than the fluctuations that can occur on tip.

        Thanks for looking at the patch.

        Show
        Aalok Shah added a comment - It appears in the current stable release (r57.1) of Go, http.Get() returns 3 values. In the latest development version, http.Get() returns 2 values. As there is no way to perform conditional compilations (a la #ifdef in C), a version can either work with the stable release or tip, but not both. I assume the Thrift project prefers working with the latest stable release rather than the fluctuations that can occur on tip. Thanks for looking at the patch.
        Hide
        Trevor Summers Smith added a comment -

        The first patch submitted has one bug that prevents the Cassandra thrift file from compiling (http.Get now returns three values and was returning two). I have attached the full patch with this bug fixed.

        I used: 6g version release.r57.1 8294 and compiled the Cassandra-0.8.0 Thrift file successfully, and tested some of its basic functionality.

        I did not do an in-depth review of all changes in this patch, because I am not yet familiar enough with the codebase.

        Show
        Trevor Summers Smith added a comment - The first patch submitted has one bug that prevents the Cassandra thrift file from compiling (http.Get now returns three values and was returning two). I have attached the full patch with this bug fixed. I used: 6g version release.r57.1 8294 and compiled the Cassandra-0.8.0 Thrift file successfully, and tested some of its basic functionality. I did not do an in-depth review of all changes in this patch, because I am not yet familiar enough with the codebase.
        Hide
        Bryan Duxbury added a comment -

        Anyone reading this feel like they can give this a review?

        Show
        Bryan Duxbury added a comment - Anyone reading this feel like they can give this a review?
        Hide
        Aalok Shah added a comment -

        Works with the latest version of Go as of May 16, 2011.

        I don't have commit access, can someone else please confirm nothing broke and commit this? Thank You.

        Show
        Aalok Shah added a comment - Works with the latest version of Go as of May 16, 2011. I don't have commit access, can someone else please confirm nothing broke and commit this? Thank You.

          People

          • Assignee:
            Aalok Shah
            Reporter:
            Aalok Shah
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development