tchclient - Add gauntlet to runThrift #162
Conversation
| { | ||
| Function: "TestEnum", | ||
| Details: "MyNumberz", | ||
| // TODO is this a thrift-gen bug? |
There was a problem hiding this comment.
Not a thrift-gen bug. This is how Apache Thrift generates code for enum constants.
| "github.com/yarpc/yarpc-go/crossdock/client/random" | ||
| "github.com/yarpc/yarpc-go/crossdock/thrift/gen-go/echo" | ||
| "github.com/yarpc/yarpc-go/crossdock/thrift/gen-go/gauntlet_apache" |
There was a problem hiding this comment.
What governs whether you choose to use underscore, hyphen, or no delimiter at all?
There was a problem hiding this comment.
the leaf package name cannot contain hypens, but directories can. The default output from Apache thrift creates gen-go hence the hyphen (internally, we prefer to use .gen/go).
Ideally package names do not have underscores, but this comes from the filename.
Idiomatic go code doesn't use hypens or underscore normally.
There was a problem hiding this comment.
No separators are preferred in general. If a separator is needed, underscores have the advantage that they can be part of the package name, so the base name and the package name match. Hyphens cannot be part of the package name so if they're used in the base name, there must be some obvious convention mapping the base name to the package name (yarpc-go => yarpc, thriftrw-go => thriftrw, foo-bar => foo_bar).
There was a problem hiding this comment.
Oh, right, in this case it is generated based on the IDL name. Unavoidable?
There was a problem hiding this comment.
Unavoidable without renaming the file. You can maybe use the namespace directive or one of the thrift compiler options, but I'm not sure.
|
LGTM |
| @@ -39,21 +44,377 @@ func runThrift(t crossdock.T, call call) { | |||
| } | |||
| token := random.String(5) | |||
|
|
|||
| call.Channel.Peers().Add(call.ServerHostPort) | |||
There was a problem hiding this comment.
optional: instead of calling Peers().Add here, you can instead pass an option to thrift.NewClient,
thrift.NewClient(call.Channel, serverName, &thrift.CallOptions{HostPort: hostPort})There was a problem hiding this comment.
Had no idea, thanks!
There was a problem hiding this comment.
Good tip, updated.
|
This is great! Suggest updating the title to have more details for the PR. lgtm |
|
LG™ |
HelloGrayson
changed the title
HelloGrayson
changed the title
| var ( | ||
| err error | ||
| got interface{} | ||
| ) |
There was a problem hiding this comment.
@abhinav @prashantv is it better to just used named return values here rather than a factored var declaration?
There was a problem hiding this comment.
Yeah, you can use named returns here.
Now that nodes represent individual constructors in the graph rather than the types they provide, we can track on the node whether it was already called. This has no value right now but we will need this once we introduce value groups to ensure that constructors don't get called multiple times.
crossdock/client/gauntlet/behavior.goto be reusable.crossdock/client/tchclient/thrift.goto include the gauntlet.@yarpc/golang