Thrift
  1. Thrift
  2. THRIFT-160

Created THttpTransport for the C# library based on WebHttpRequest

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 0.4
    • Component/s: C# - Library
    • Labels:
      None
    • Environment:

      .NET C#

    • Patch Info:
      Patch Available

      Description

      I have created and done some testing of a HttpTransport for the C# library. I started with the Java implementation and ported/created a C# version of it based on the WebHttpRequest object in the .NET BCLs.

      1. THttpClient.cs
        5 kB
        Brian O'Neil
      2. THttpClient.cs
        5 kB
        Todd Gardner
      3. client.patch
        1.0 kB
        Todd Gardner
      4. THRIFT-160-v3.diff
        6 kB
        Michael Greene

        Activity

        Hide
        Brian O'Neil added a comment -

        THttpTransport for the C# library based on WebHttpRequest

        Show
        Brian O'Neil added a comment - THttpTransport for the C# library based on WebHttpRequest
        Hide
        Michael Greene added a comment -

        This agent should identify itself as something other than "Java/THttpClient"

        The coding style is a little different than other Thrift C# code.
        For example, see the empty method and simple property styles on Open and IsOpen in TStreamTransport.cs versus in the attached file.

        Tabs/spaces are different as well – I'm not sure how important that is.

        The checks for null streams could pass TTransportException.ExceptionType.NotOpen as the first parameter of the constructed TTransportException.

        The header should probably reference the Apache site as opposed to the Facebook one

        Show
        Michael Greene added a comment - This agent should identify itself as something other than "Java/THttpClient" The coding style is a little different than other Thrift C# code. For example, see the empty method and simple property styles on Open and IsOpen in TStreamTransport.cs versus in the attached file. Tabs/spaces are different as well – I'm not sure how important that is. The checks for null streams could pass TTransportException.ExceptionType.NotOpen as the first parameter of the constructed TTransportException. The header should probably reference the Apache site as opposed to the Facebook one
        Hide
        Michael Greene added a comment -

        Is there a reason for doing the conversion to System.Uri internal to the object on construction? It seems more in keeping with .NET, and ever so slightly more performant, to just accept the Uri itself.

        Additionally, this would remove the need for a try/catch block (if there was a need to begin with) in the constructor.

        Show
        Michael Greene added a comment - Is there a reason for doing the conversion to System.Uri internal to the object on construction? It seems more in keeping with .NET, and ever so slightly more performant, to just accept the Uri itself. Additionally, this would remove the need for a try/catch block (if there was a need to begin with) in the constructor.
        Hide
        Brian O'Neil added a comment -

        Sorry, I did a quick port of the Java THttpClient class because I needed it to be able to do something and thought that the work would be useful.

        Most of the comment relate to things that are put together from the Java one. I will try to come back and make some updates to it as soon as I get a chance.

        Show
        Brian O'Neil added a comment - Sorry, I did a quick port of the Java THttpClient class because I needed it to be able to do something and thought that the work would be useful. Most of the comment relate to things that are put together from the Java one. I will try to come back and make some updates to it as soon as I get a chance.
        Hide
        Bryan Duxbury added a comment -

        Is there going to be any more attention paid to this issue? It's been open for a long time and inactive for most of that time.

        Show
        Bryan Duxbury added a comment - Is there going to be any more attention paid to this issue? It's been open for a long time and inactive for most of that time.
        Hide
        Michael Greene added a comment -

        I'm hoping Brian or someone else who needs this will respond, but otherwise I'd be interested in completing this to flesh out the library.

        Show
        Michael Greene added a comment - I'm hoping Brian or someone else who needs this will respond, but otherwise I'd be interested in completing this to flesh out the library.
        Hide
        Todd Gardner added a comment -

        I recently had need of the same thing, so I adjusted the patch above with the suggestions, as well as some general cleanup. I also tried to override the Peek method, but it looks like it isn't virtual in TTransport.cs, which differs from the c++ version (not sure which one is correct).

        I've also included a patch for the client, such that when the URL parameter is passed on the command line it uses THTTPClient.cs. The URL parameter didn't do anything before, as far as I could tell.

        Show
        Todd Gardner added a comment - I recently had need of the same thing, so I adjusted the patch above with the suggestions, as well as some general cleanup. I also tried to override the Peek method, but it looks like it isn't virtual in TTransport.cs, which differs from the c++ version (not sure which one is correct). I've also included a patch for the client, such that when the URL parameter is passed on the command line it uses THTTPClient.cs. The URL parameter didn't do anything before, as far as I could tell.
        Hide
        Anatoly Fayngelerin added a comment -

        I have been using this patch for some time now without any problems. Are there any outstanding fixes that need to be made before the patch is integrated into the codebase? If there are, I would be happy to complete any work that is required for getting this integrated.

        Show
        Anatoly Fayngelerin added a comment - I have been using this patch for some time now without any problems. Are there any outstanding fixes that need to be made before the patch is integrated into the codebase? If there are, I would be happy to complete any work that is required for getting this integrated.
        Hide
        Bryan Duxbury added a comment -

        Unless one of the CSharp committers wants to take care of this, I'm happy to commit it. Just to be clear, you want the latest version of the patch committed with no modifications?

        Show
        Bryan Duxbury added a comment - Unless one of the CSharp committers wants to take care of this, I'm happy to commit it. Just to be clear, you want the latest version of the patch committed with no modifications?
        Hide
        Michael Greene added a comment -

        There are no C# committers, but Todd's latest version of the file and test patch can get a +1 from me, excluding the attribution beneath the license.

        Show
        Michael Greene added a comment - There are no C# committers, but Todd's latest version of the file and test patch can get a +1 from me, excluding the attribution beneath the license.
        Hide
        James E. King, III added a comment -

        You also need to patch up the Visual Studio project to include the file(s).

        Show
        James E. King, III added a comment - You also need to patch up the Visual Studio project to include the file(s).
        Hide
        Michael Greene added a comment -

        This patch makes spaces/tabs conform with the rest of the Thrift C# code, removes the author attribution, fixes some CRLF inconsistency, and adds the THttpClient to both the Makefile and the csproj file.

        It replaces the previous .cs file and .patch

        Show
        Michael Greene added a comment - This patch makes spaces/tabs conform with the rest of the Thrift C# code, removes the author attribution, fixes some CRLF inconsistency, and adds the THttpClient to both the Makefile and the csproj file. It replaces the previous .cs file and .patch
        Hide
        Bryan Duxbury added a comment -

        here are no C# committers

        Really? Why? We should remedy that. Any volunteers?

        I'll commit Michael's latest patch if there are no objections.

        Show
        Bryan Duxbury added a comment - here are no C# committers Really? Why? We should remedy that. Any volunteers? I'll commit Michael's latest patch if there are no objections.
        Hide
        Bryan Duxbury added a comment -

        I just committed this.

        Show
        Bryan Duxbury added a comment - I just committed this.

          People

          • Assignee:
            Michael Greene
            Reporter:
            Brian O'Neil
          • Votes:
            0 Vote for this issue
            Watchers:
            4 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development