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.diff
        22 kB
        Aalok Shah
      2. gopatch2-r2.diff
        22 kB
        Trevor Summers Smith

        Activity

        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.
        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
        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
        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
        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
        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 -

        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 -

        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
        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

          People

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

            Dates

            • Created:
              Updated:
              Resolved:

              Development