Thrift
  1. Thrift
  2. THRIFT-574

erlang socket servers should have an option to use a framed transport

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 0.1
    • Fix Version/s: 0.2
    • Component/s: Erlang - Library
    • Labels:
      None
    • Environment:

      erlang R13B01

    • Patch Info:
      Patch Available

      Description

      While it is simple to construct an erlang thrift client that uses a framed transport, there is no simple option to do the same with an erlang thrift server.
      I'm attaching a patch that adds this functionality.

        Activity

        Hide
        Ben Taitelbaum added a comment -

        This patch allows you to construct a server with the framed option, for example:

        thrift_socket_server:start([{handler, ?MODULE},
                                    {service, calculator_thrift},
                                    {port, 9090},
                                    {name, tutorial_server},
                                    {framed, true}]).
        
        Show
        Ben Taitelbaum added a comment - This patch allows you to construct a server with the framed option, for example: thrift_socket_server:start([{handler, ?MODULE}, {service, calculator_thrift}, {port, 9090}, {name, tutorial_server}, {framed, true}]).
        Hide
        David Reiss added a comment -

        I assume you meant to delete this line:

        {ok, BufferedTransport}

        = thrift_buffered_transport:new(SocketTransport),

        Otherwise, LGTM modulo some whitespace issues. Todd?

        Show
        David Reiss added a comment - I assume you meant to delete this line: {ok, BufferedTransport} = thrift_buffered_transport:new(SocketTransport), Otherwise, LGTM modulo some whitespace issues. Todd?
        Hide
        Ben Taitelbaum added a comment -

        I did mean to delete that line. Sorry about that. Here's a new patch, with more consistent whitespace as well.

        Show
        Ben Taitelbaum added a comment - I did mean to delete that line. Sorry about that. Here's a new patch, with more consistent whitespace as well.
        Hide
        Ben Taitelbaum added a comment -

        removed unnecessary line and fixed whitespace issues.

        Show
        Ben Taitelbaum added a comment - removed unnecessary line and fixed whitespace issues.
        Hide
        Todd Lipcon added a comment -

        Small style nits:

        • Still seems to be a tab introduced in the record definition
        • I'd like to see an is_boolean guard for the parse_options clause
        • Similarly, instead of "case Framed of true -> ...; _ -> ... end" use "false" for the second clause

        Aside from that, looks good, assuming you've done some tests and it works (I've never tested a framed server in Erlang). Thanks, Ben!

        Show
        Todd Lipcon added a comment - Small style nits: Still seems to be a tab introduced in the record definition I'd like to see an is_boolean guard for the parse_options clause Similarly, instead of "case Framed of true -> ...; _ -> ... end" use "false" for the second clause Aside from that, looks good, assuming you've done some tests and it works (I've never tested a framed server in Erlang). Thanks, Ben!
        Hide
        Ben Taitelbaum added a comment -

        I thought I had configured emacs to replace tabs with whitespace when saving, but I'll take another look.

        I see your point about true/false as opposed to true/_ – it's better to get a badmatch error than to match on, say, '1' (although that won't happen when I add the is_boolean guard).

        I'll make these changes and upload a new patch.

        Show
        Ben Taitelbaum added a comment - I thought I had configured emacs to replace tabs with whitespace when saving, but I'll take another look. I see your point about true/false as opposed to true/_ – it's better to get a badmatch error than to match on, say, '1' (although that won't happen when I add the is_boolean guard). I'll make these changes and upload a new patch.
        Show
        Ben Taitelbaum added a comment - Made changes from comment https://issues.apache.org/jira/browse/THRIFT-574?focusedCommentId=12751569&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#action_12751569
        Hide
        Ben Taitelbaum added a comment -

        I made those changes, and then ran the whole buffer through a little M-x untabify, so this latest patch should be good.

        Show
        Ben Taitelbaum added a comment - I made those changes, and then ran the whole buffer through a little M-x untabify, so this latest patch should be good.
        Hide
        Todd Lipcon added a comment -

        +1 lgtm

        Show
        Todd Lipcon added a comment - +1 lgtm
        Hide
        Ben Taitelbaum added a comment -

        Thanks!

        Show
        Ben Taitelbaum added a comment - Thanks!

          People

          • Assignee:
            Ben Taitelbaum
            Reporter:
            Ben Taitelbaum
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development