Merge pull request #91 from yarpc/dev

Release latest yab changes as 0.6.0
yarpc · Aug 1, 2016 · c3092bc · c3092bc
2 parents 342842c + 834346c
commit c3092bc
Show file tree

Hide file tree

Showing 29 changed files with 367 additions and 206 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -0,0 +1,30 @@
+Changelog
+=========
+
+# 0.6.0
+
+* Allow JSON to be used with non-map requests and responses.
+* Expose HTTP response body on non-OK responses.
+* Expose TChannel call status ("ok") and HTTP status code ("code"
+  on successful responses.
+* Fix ini parsing of option groups. (#82)
+* Add support for TMultiplexedProtocol when using HTTP+Thrift using
+  `--multiplexed-thrift`.
+
+# 0.5.4
+
+* Fix for benchmarking taking longer than duration at low RPS. (#73)
+
+# 0.5.2
+
+* Fix `--peer-list` not loading the peer list. Regression from 0.5.0. (#70)
+
+# 0.5.0
+
+* Support for reading default options using XDG base directories.
+* Round robin peer-selection when creating connections for benchmarking.
+* Allow disabling Thrift envelopes for HTTP using `--disable-thrift-envelope`.
+
+# 0.4.0
+
+* First beta release.
diff --git a/Makefile b/Makefile
@@ -29,7 +29,7 @@ install_ci: install
 .PHONY: update_man
 update_man:
 	go install .
-	yab --man-page > man/yab.1
+	$$GOPATH/bin/yab --man-page > man/yab.1
 	groff -man -T html man/yab.1 > man/yab.html
 	[[ -d ../yab_ghpages ]] && cp man/yab.html ../yab_ghpages/man.html
 	@echo "Please update gh-pages"

diff --git a/README.md b/README.md
@@ -22,7 +22,6 @@ This will install `yab` to `$GOPATH/bin/yab`.
 Usage:
   yab [<service> <method> <body>] [OPTIONS]
 
-
 Application Options:
       --version                  Displays the application version
 
@@ -45,6 +44,9 @@ Request Options:
                                  milliseconds are assumed. (default: 1s)
       --disable-thrift-envelope  Disables Thrift envelopes (disabled by default
                                  for TChannel)
+      --multiplexed-thrift       Enables the Thrift TMultiplexedProtocol used
+                                 by services that host multiple Thrift services
+                                 on a single endpoint.
 
 Transport Options:
   -s, --service=                 The TChannel/Hyperbahn service name

diff --git a/encoding/encoding_test.go b/encoding/encoding_test.go
@@ -143,7 +143,7 @@ func TestJSONEncodingResponse(t *testing.T) {
 
 	tests := []struct {
 		data   string
-		want   map[string]interface{}
+		want   interface{}
 		errMsg string
 	}{
 		{
@@ -160,6 +160,14 @@ func TestJSONEncodingResponse(t *testing.T) {
 			}`,
 			want: map[string]interface{}{"key": json.Number("123")},
 		},
+		{
+			data: `1`,
+			want: json.Number("1"),
+		},
+		{
+			data: `"hello world"`,
+			want: "hello world",
+		},
 	}
 
 	for _, tt := range tests {

diff --git a/encoding/thrift_request.go b/encoding/thrift_request.go
@@ -33,6 +33,8 @@ import (
 	"github.com/thriftrw/thriftrw-go/compile"
 )
 
+const _multiplexedSeparator = ":"
+
 var defaultOpts = thrift.Options{UseEnvelopes: true}
 
 type thriftSerializer struct {
@@ -42,7 +44,7 @@ type thriftSerializer struct {
 }
 
 // NewThrift returns a Thrift serializer.
-func NewThrift(thriftFile, methodName string) (Serializer, error) {
+func NewThrift(thriftFile, methodName string, multiplexed bool) (Serializer, error) {
 	if thriftFile == "" {
 		return nil, errors.New("specify a Thrift file using --thrift")
 	}
@@ -70,7 +72,12 @@ func NewThrift(thriftFile, methodName string) (Serializer, error) {
 		return nil, err
 	}
 
-	return thriftSerializer{methodName, spec, defaultOpts}, nil
+	opts := defaultOpts
+	if multiplexed {
+		opts.EnvelopeMethodPrefix = thriftSvc + _multiplexedSeparator
+	}
+
+	return thriftSerializer{methodName, spec, opts}, nil
 }
 
 func (e thriftSerializer) Encoding() Encoding {

diff --git a/encoding/thrift_request_test.go b/encoding/thrift_request_test.go
@@ -38,6 +38,7 @@ func TestNewThriftSerializer(t *testing.T) {
 	tests := []struct {
 		desc         string
 		file, method string
+		multiplexed  bool
 		errMsg       string
 		errMsgs      []string
 	}{
@@ -78,10 +79,16 @@ func TestNewThriftSerializer(t *testing.T) {
 			file:   validThrift,
 			method: fooMethod,
 		},
+		{
+			desc:        "Valid Thrift file and method name multiplexed",
+			file:        validThrift,
+			method:      fooMethod,
+			multiplexed: true,
+		},
 	}
 
 	for _, tt := range tests {
-		got, err := NewThrift(tt.file, tt.method)
+		got, err := NewThrift(tt.file, tt.method, tt.multiplexed)
 		if tt.errMsg == "" {
 			assert.NoError(t, err, "%v", tt.desc)
 			if assert.NotNil(t, got, "%v: Invalid request") {
@@ -98,7 +105,7 @@ func TestNewThriftSerializer(t *testing.T) {
 }
 
 func TestRequest(t *testing.T) {
-	serializer, err := NewThrift(validThrift, "Simple::foo")
+	serializer, err := NewThrift(validThrift, "Simple::foo", false /* multiplexed */)
 	require.NoError(t, err, "Failed to create serializer")
 
 	tests := []struct {
@@ -245,17 +252,14 @@ func TestFindMethod(t *testing.T) {
 }
 
 func TestWithoutEnvelopes(t *testing.T) {
-	serializer, err := NewThrift(validThrift, "Simple::foo")
-	require.NoError(t, err, "Failed to create serializer")
-
 	tests := []struct {
-		desc       string
-		serializer Serializer
-		want       []byte
+		desc             string
+		multiplexed      bool
+		withoutEnvelopes bool
+		want             []byte
 	}{
 		{
-			desc:       "with envelope",
-			serializer: serializer,
+			desc: "with envelope",
 			want: []byte{
 				0x80, 0x01, 0x00, 0x01, // version | type = 1 | call
 				0x00, 0x00, 0x00, 0x03, 'f', 'o', 'o', // "foo"
@@ -264,14 +268,38 @@ func TestWithoutEnvelopes(t *testing.T) {
 			},
 		},
 		{
-			desc:       "without envelope",
-			serializer: serializer.(thriftSerializer).WithoutEnvelopes(),
-			want:       []byte{0x00},
+			desc:        "with envelope, multiplexed",
+			multiplexed: true,
+			want: []byte{
+				0x80, 0x01, 0x00, 0x01, // version | type = 1 | call
+				0x00, 0x00, 0x00, 0x0A, // length of method
+				'S', 'i', 'm', 'p', 'l', 'e', ':', 'f', 'o', 'o',
+				0x00, 0x00, 0x00, 0x00, // seqID
+				0x00, // empty struct
+			},
+		},
+		{
+			desc:             "without envelope",
+			withoutEnvelopes: true,
+			want:             []byte{0x00},
+		},
+		{
+			desc:             "without envelope, multiplexed",
+			multiplexed:      true, // has no effect when there are no envelopes.
+			withoutEnvelopes: true,
+			want:             []byte{0x00},
 		},
 	}
 
 	for _, tt := range tests {
-		got, err := tt.serializer.Request([]byte("{}"))
+		serializer, err := NewThrift(validThrift, "Simple::foo", tt.multiplexed)
+		require.NoError(t, err, "Failed to create serializer")
+
+		if tt.withoutEnvelopes {
+			serializer = serializer.(thriftSerializer).WithoutEnvelopes()
+		}
+
+		got, err := serializer.Request([]byte("{}"))
 		require.NoError(t, err, "%v: serialize failed", tt.desc)
 		assert.Equal(t, tt.want, got.Body, "%v: got unexpected bytes", tt.desc)
 	}

diff --git a/glide.lock b/glide.lock
diff --git a/glide.yaml b/glide.yaml
@@ -3,7 +3,7 @@ import:
 - package: github.com/jessevdk/go-flags
   version: master
 - package: github.com/thriftrw/thriftrw-go
-  version: d511d45b9dbe1e412b663ca6186118ad52da2fb8
+  version: 13abcf75760f008b6f92ee3185a9534e286b64a0
   subpackages:
   - ast
   - compile

diff --git a/integration_test.go b/integration_test.go
@@ -81,25 +81,47 @@ func (httpHandler) Bar(arg int32) (int32, error) {
 }
 
 func TestIntegrationProtocols(t *testing.T) {
-	ch := setupTChannelIntegrationServer(t)
-	defer ch.Close()
-
-	httpServer := setupHTTPIntegrationServer(t)
-	defer httpServer.Close()
-
-	tests := []string{
-		ch.PeerInfo().HostPort,
-		httpServer.URL,
+	cases := []struct {
+		desc        string
+		setup       func() (hostPort string, shutdown func())
+		multiplexed bool
+	}{
+		{
+			desc: "TChannel",
+			setup: func() (hostPort string, shutdown func()) {
+				ch := setupTChannelIntegrationServer(t)
+				return ch.PeerInfo().HostPort, ch.Close
+			},
+		},
+		{
+			desc: "Non-multiplexed HTTP",
+			setup: func() (hostPort string, shutdown func()) {
+				httpServer := setupHTTPIntegrationServer(t, false /* multiplexed */)
+				return httpServer.URL, httpServer.Close
+			},
+		},
+		{
+			desc: "Multiplexed HTTP",
+			setup: func() (hostPort string, shutdown func()) {
+				httpServer := setupHTTPIntegrationServer(t, true /* multiplexed */)
+				return httpServer.URL, httpServer.Close
+			},
+			multiplexed: true,
+		},
 	}
 
-	for _, peer := range tests {
+	for _, c := range cases {
+		peer, shutdown := c.setup()
+		defer shutdown()
+
 		for _, tt := range integrationTests {
 			opts := Options{
 				ROpts: RequestOptions{
-					ThriftFile:  "testdata/integration.thrift",
-					MethodName:  "Foo::bar",
-					Timeout:     timeMillisFlag(time.Second),
-					RequestJSON: fmt.Sprintf(`{"arg": %v}`, tt.call),
+					ThriftFile:        "testdata/integration.thrift",
+					MethodName:        "Foo::bar",
+					Timeout:           timeMillisFlag(time.Second),
+					RequestJSON:       fmt.Sprintf(`{"arg": %v}`, tt.call),
+					ThriftMultiplexed: c.multiplexed,
 				},
 				TOpts: TransportOptions{
 					ServiceName: "foo",
@@ -108,8 +130,8 @@ func TestIntegrationProtocols(t *testing.T) {
 			}
 
 			gotOut, gotErr := runTestWithOpts(opts)
-			assert.Contains(t, gotOut, tt.wantRes, "Unexpected result for %v", tt.call)
-			assert.Contains(t, gotErr, tt.wantErr, "Unexpected error for %v", tt.call)
+			assert.Contains(t, gotOut, tt.wantRes, "%v: Unexpected result for %v", c.desc, tt.call)
+			assert.Contains(t, gotErr, tt.wantErr, "%v: Unexpected error for %v", c.desc, tt.call)
 		}
 	}
 }
@@ -146,9 +168,16 @@ func setupTChannelIntegrationServer(t *testing.T) *tchannel.Channel {
 	return ch
 }
 
-func setupHTTPIntegrationServer(t *testing.T) *httptest.Server {
+func setupHTTPIntegrationServer(t *testing.T, multiplexed bool) *httptest.Server {
+	var processor athrift.TProcessor = integration.NewFooProcessor(httpHandler{})
 	protocolFactory := athrift.NewTBinaryProtocolFactoryDefault()
-	processor := integration.NewFooProcessor(httpHandler{})
+
+	if multiplexed {
+		multiProcessor := athrift.NewTMultiplexedProcessor()
+		multiProcessor.RegisterProcessor("Foo", processor)
+		processor = multiProcessor
+	}
+
 	handler := athrift.NewThriftHandlerFunc(processor, protocolFactory, protocolFactory)
 	return httptest.NewServer(http.HandlerFunc(handler))
 }
diff --git a/main.go b/main.go
@@ -117,6 +117,11 @@ Default options can be specified in a ~/.config/yab/defaults.ini file with conte
 	warmup = 10
 `
 
+	// Read defaults if they're available, before we change the group names.
+	if err := parseDefaultConfigs(parser); err != nil {
+		return nil, fmt.Errorf("error reading defaults: %v", err)
+	}
+
 	setGroupDescs(parser, "request", "Request Options", toGroff(_reqOptsDesc))
 	setGroupDescs(parser, "transport", "Transport Options", toGroff(_transportOptsDesc))
 	setGroupDescs(parser, "benchmark", "Benchmark Options", toGroff(_benchmarkOptsDesc))
@@ -127,11 +132,6 @@ Default options can be specified in a ~/.config/yab/defaults.ini file with conte
 		return opts, errExit
 	}
 
-	// Read defaults if they're available.
-	if err := parseDefaultConfigs(parser); err != nil {
-		return nil, fmt.Errorf("error reading defaults: %v\n", err)
-	}
-
 	remaining, err := parser.ParseArgs(args)
 	if err != nil {
 		if ferr, ok := err.(*flags.Error); ok {
@@ -255,8 +255,8 @@ func runWithOptions(opts Options, out output) {
 	if len(response.Headers) > 0 {
 		outSerialized["headers"] = response.Headers
 	}
-	if response.Trace != "" {
-		outSerialized["trace"] = response.Trace
+	for k, v := range response.TransportFields {
+		outSerialized[k] = v
 	}
 	bs, err := json.MarshalIndent(outSerialized, "", "  ")
 	if err != nil {
@@ -279,7 +279,7 @@ type noEnveloper interface {
 func withTransportSerializer(p transport.Protocol, s encoding.Serializer, rOpts RequestOptions) encoding.Serializer {
 	switch {
 	case p == transport.TChannel && s.Encoding() == encoding.Thrift,
-		rOpts.DisableThriftEnvelopes:
+		rOpts.ThriftDisableEnvelopes:
 		s = s.(noEnveloper).WithoutEnvelopes()
 	}
 	return s