Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

THRIFT-4914: Make TClient.Call to return the response meta #2315

Merged
merged 1 commit into from
Jan 23, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGES.md
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
- [THRIFT-5152](https://issues.apache.org/jira/browse/THRIFT-5152) - go: TSocket and TSSLSocket now have separated connect timeout and socket timeout
- c++: dropped support for Windows XP
- [THRIFT-5326](https://issues.apache.org/jira/browse/THRIFT-5326) - go: TException interface now has a new function: TExceptionType
- [THRIFT-4914](https://issues.apache.org/jira/browse/THRIFT-4914) - go: TClient.Call now returns ResponseMeta in addition to error

### Java

Expand All @@ -31,6 +32,7 @@
- [THRIFT-5233](https://issues.apache.org/jira/browse/THRIFT-5233) - Add context deadline check to ReadMessageBegin in TBinaryProtocol, TCompactProtocol, and THeaderProtocol.
- [THRIFT-5240](https://issues.apache.org/jira/browse/THRIFT-5240) - The context passed into server handler implementations will be canceled when we detected that the client closed the connection.
- [THRIFT-5322](https://issues.apache.org/jira/browse/THRIFT-5322) - Add support to TConfiguration, and also fix a bug that could cause excessive memory usage when reading malformed messages from TCompactProtocol.
- [THRIFT-4914](https://issues.apache.org/jira/browse/THRIFT-4914) - Compiler generated service clients now provide a new function, LastResponseMeta_(), to get the response metadata (e.g. headers from THeader) from the last client call.

## 0.13.0

Expand Down
38 changes: 31 additions & 7 deletions compiler/cpp/src/thrift/generate/t_go_generator.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1493,11 +1493,15 @@ void t_go_generator::generate_go_struct_definition(ostream& out,

if (is_exception) {
out << indent() << "func (p *" << tstruct_name << ") Error() string {" << endl;
out << indent() << indent() << "return p.String()" << endl;
indent_up();
out << indent() << "return p.String()" << endl;
indent_down();
out << indent() << "}" << endl << endl;

out << indent() << "func (" << tstruct_name << ") TExceptionType() thrift.TExceptionType {" << endl;
out << indent() << indent() << "return thrift.TExceptionTypeCompiled" << endl;
indent_up();
out << indent() << "return thrift.TExceptionTypeCompiled" << endl;
indent_down();
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are cosmetic fix of a wrongly did indent from my previous PR :$ not really related to this PR.

out << indent() << "}" << endl << endl;

out << indent() << "var _ thrift.TException = (*" << tstruct_name << ")(nil)"
Expand Down Expand Up @@ -1990,6 +1994,7 @@ void t_go_generator::generate_service_client(t_service* tservice) {
f_types_ << indent() << "*" << extends_client << endl;
} else {
f_types_ << indent() << "c thrift.TClient" << endl;
f_types_ << indent() << "meta thrift.ResponseMeta" << endl;
}

indent_down();
Expand Down Expand Up @@ -2059,7 +2064,19 @@ void t_go_generator::generate_service_client(t_service* tservice) {
indent_up();
f_types_ << indent() << "return p.c" << endl;
indent_down();
f_types_ << indent() << "}" << endl;
f_types_ << indent() << "}" << endl << endl;

f_types_ << indent() << "func (p *" << serviceName << "Client) LastResponseMeta_() thrift.ResponseMeta {" << endl;
indent_up();
f_types_ << indent() << "return p.meta" << endl;
indent_down();
f_types_ << indent() << "}" << endl << endl;

f_types_ << indent() << "func (p *" << serviceName << "Client) SetLastResponseMeta_(meta thrift.ResponseMeta) {" << endl;
indent_up();
f_types_ << indent() << "p.meta = meta" << endl;
indent_down();
f_types_ << indent() << "}" << endl << endl;
}

// Generate client method implementations
Expand Down Expand Up @@ -2091,8 +2108,11 @@ void t_go_generator::generate_service_client(t_service* tservice) {
std::string resultName = tmp("_result");
std::string resultType = publicize(method + "_result", true);
f_types_ << indent() << "var " << resultName << " " << resultType << endl;
f_types_ << indent() << "if err = p.Client_().Call(ctx, \""
<< method << "\", &" << argsName << ", &" << resultName << "); err != nil {" << endl;
f_types_ << indent() << "var meta thrift.ResponseMeta" << endl;
f_types_ << indent() << "meta, err = p.Client_().Call(ctx, \""
<< method << "\", &" << argsName << ", &" << resultName << ")" << endl;
f_types_ << indent() << "p.SetLastResponseMeta_(meta)" << endl;
f_types_ << indent() << "if err != nil {" << endl;
Comment on lines -2094 to +2115

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we use tmp("_meta") or something? I'm seeing meta redeclaration errors when meta is already defined in args, for example:
https://github.com/apache/storm/blob/v2.2.0/storm-client/src/storm.thrift#L752
produces the snippet below which throws meta redeclared error

// Parameters:
//  - Key
//  - Meta
func (p *NimbusClient) BeginCreateBlob(ctx context.Context, key string, meta *SettableBlobMeta) (r string, err error) {
  var _args293 NimbusBeginCreateBlobArgs
  _args293.Key = key
  _args293.Meta = meta
  var _result294 NimbusBeginCreateBlobResult
  var meta thrift.ResponseMeta
  meta, err = p.Client_().Call(ctx, "beginCreateBlob", &_args293, &_result294)
  p.SetLastResponseMeta_(meta)
  if err != nil {
    return
  }
  switch {
  case _result294.Aze!= nil:
    return r, _result294.Aze
  case _result294.Kae!= nil:
    return r, _result294.Kae
  }

  return _result294.GetSuccess(), nil
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. Thanks for reporting. Preparing a PR to fix it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.


indent_up();
f_types_ << indent() << "return" << endl;
Expand Down Expand Up @@ -2131,9 +2151,13 @@ void t_go_generator::generate_service_client(t_service* tservice) {
f_types_ << indent() << "return nil" << endl;
}
} else {
// Since we don't have response meta for oneway calls, overwrite it with
// an empty one to avoid users getting the meta from last call and
// mistaken it as from the oneway call.
f_types_ << indent() << "p.SetLastResponseMeta_(thrift.ResponseMeta{})" << endl;
// TODO: would be nice to not to duplicate the call generation
f_types_ << indent() << "if err := p.Client_().Call(ctx, \""
<< method << "\", &"<< argsName << ", nil); err != nil {" << endl;
f_types_ << indent() << "if _, err := p.Client_().Call(ctx, \""
<< method << "\", &"<< argsName << ", nil); err != nil {" << endl;

indent_up();
f_types_ << indent() << "return err" << endl;
Expand Down
24 changes: 19 additions & 5 deletions lib/go/thrift/client.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,15 @@ import (
"fmt"
)

// ResponseMeta represents the metadata attached to the response.
type ResponseMeta struct {
// The headers in the response, if any.
// If the underlying transport/protocol is not THeader, this will always be nil.
Headers THeaderMap
}

type TClient interface {
Call(ctx context.Context, method string, args, result TStruct) error
Call(ctx context.Context, method string, args, result TStruct) (ResponseMeta, error)
}

type TStandardClient struct {
Expand Down Expand Up @@ -78,18 +85,25 @@ func (p *TStandardClient) Recv(ctx context.Context, iprot TProtocol, seqId int32
return iprot.ReadMessageEnd(ctx)
}

func (p *TStandardClient) Call(ctx context.Context, method string, args, result TStruct) error {
func (p *TStandardClient) Call(ctx context.Context, method string, args, result TStruct) (ResponseMeta, error) {
p.seqId++
seqId := p.seqId

if err := p.Send(ctx, p.oprot, seqId, method, args); err != nil {
return err
return ResponseMeta{}, err
}

// method is oneway
if result == nil {
return nil
return ResponseMeta{}, nil
}

return p.Recv(ctx, p.iprot, seqId, method, result)
err := p.Recv(ctx, p.iprot, seqId, method, result)
var headers THeaderMap
if hp, ok := p.iprot.(*THeaderProtocol); ok {
headers = hp.transport.readHeaders
}
return ResponseMeta{
Headers: headers,
}, err
}
6 changes: 3 additions & 3 deletions lib/go/thrift/example_client_middleware_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -45,16 +45,16 @@ func NewMyServiceClient(_ TClient) MyService {

func simpleClientLoggingMiddleware(next TClient) TClient {
return WrappedTClient{
Wrapped: func(ctx context.Context, method string, args, result TStruct) error {
Wrapped: func(ctx context.Context, method string, args, result TStruct) (ResponseMeta, error) {
log.Printf("Before: %q", method)
log.Printf("Args: %#v", args)
err := next.Call(ctx, method, args, result)
headers, err := next.Call(ctx, method, args, result)
log.Printf("After: %q", method)
log.Printf("Result: %#v", result)
if err != nil {
log.Printf("Error: %v", err)
}
return err
return headers, err
},
}
}
Expand Down
14 changes: 0 additions & 14 deletions lib/go/thrift/header_protocol.go
Original file line number Diff line number Diff line change
Expand Up @@ -345,20 +345,6 @@ func (p *THeaderProtocol) SetTConfiguration(cfg *TConfiguration) {
p.cfg = cfg
}

// GetResponseHeadersFromClient is a helper function to get the read THeaderMap
// from the last response received from the given client.
//
// If the last response was not sent over THeader protocol,
// a nil map will be returned.
func GetResponseHeadersFromClient(c TClient) THeaderMap {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function was in a tagged release. Maybe mark it as deprecated instead of removing?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was added after 0.13.0 so I don't believe it's in any tagged release :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I was confused because the commit date was earlier than the 0.13 tag :)

if sc, ok := c.(*TStandardClient); ok {
if hp, ok := sc.iprot.(*THeaderProtocol); ok {
return hp.transport.readHeaders
}
}
return nil
}

var (
_ TConfigurationSetter = (*tHeaderProtocolFactory)(nil)
_ TConfigurationSetter = (*THeaderProtocol)(nil)
Expand Down
4 changes: 2 additions & 2 deletions lib/go/thrift/middleware.go
Original file line number Diff line number Diff line change
Expand Up @@ -78,11 +78,11 @@ type ClientMiddleware func(TClient) TClient
//
// This is provided to aid in developing ClientMiddleware.
type WrappedTClient struct {
Wrapped func(ctx context.Context, method string, args, result TStruct) error
Wrapped func(ctx context.Context, method string, args, result TStruct) (ResponseMeta, error)
}

// Call implements the TClient interface by calling and returning c.Wrapped.
func (c WrappedTClient) Call(ctx context.Context, method string, args, result TStruct) error {
func (c WrappedTClient) Call(ctx context.Context, method string, args, result TStruct) (ResponseMeta, error) {
return c.Wrapped(ctx, method, args, result)
}

Expand Down
6 changes: 3 additions & 3 deletions lib/go/thrift/middleware_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ func testProcessorMiddleware(c *counter) ProcessorMiddleware {
func testClientMiddleware(c *counter) ClientMiddleware {
return func(next TClient) TClient {
return WrappedTClient{
Wrapped: func(ctx context.Context, method string, args, result TStruct) error {
Wrapped: func(ctx context.Context, method string, args, result TStruct) (ResponseMeta, error) {
c.incr()
return next.Call(ctx, method, args, result)
},
Expand Down Expand Up @@ -122,8 +122,8 @@ func TestWrapTMultiplexedProcessor(t *testing.T) {

func TestWrapClient(t *testing.T) {
client := WrappedTClient{
Wrapped: func(ctx context.Context, method string, args, result TStruct) error {
return nil
Wrapped: func(ctx context.Context, method string, args, result TStruct) (ResponseMeta, error) {
return ResponseMeta{}, nil
},
}
c := newCounter(t)
Expand Down