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

fishy · 2021-01-22T17:38:51Z

Client: go

Make a breaking change so that TClient.Call returns the response
meta, currently only contains headers but could be expanded in the
future, and make a compiler change to compiler generated clients to take
advantage of that and provide access to response metadata to users.

dcelasun · 2021-01-22T17:45:38Z

lib/go/thrift/header_protocol.go

-//
-// If the last response was not sent over THeader protocol,
-// a nil map will be returned.
-func GetResponseHeadersFromClient(c TClient) THeaderMap {


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

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

At least it's not here: https://pkg.go.dev/github.com/apache/[email protected]/lib/go/thrift :)

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

fishy · 2021-01-22T17:53:26Z

compiler/cpp/src/thrift/generate/t_go_generator.cc

-    out << indent() << indent() << "return thrift.TExceptionTypeCompiled" << endl;
+    indent_up();
+    out << indent() << "return thrift.TExceptionTypeCompiled" << endl;
+    indent_down();


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

fishy · 2021-01-22T18:53:04Z

@dcelasun On second thought, I changed it from THeaderMap to a struct ResponseMeta containing THeaderMap, so it can be expanded in the future to add more stuff and that's no longer a breaking change.

Client: go Make a breaking change so that TClient.Call returns the response meta, currently only contains headers but could be expanded in the future, and make a compiler change to compiler generated clients to take advantage of that and provide access to response metadata to users.

yhlee-tw · 2021-02-17T20:25:21Z

compiler/cpp/src/thrift/generate/t_go_generator.cc

-      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;


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 }

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

Client: go This fixes the bug reported in apache#2315 (comment).

Client: go This fixes the bug reported in #2315 (comment).

Client: go This fixes the bug reported in apache#2315 (comment).

Client: go This fixes the bug reported in #2315 (comment).

litao3rd · 2021-07-23T07:42:35Z

hello, in the latest version of thrift golang library, when I compile gen-go code with go build, I got error like this

gen-go/package/XXXX.go:1353:11: cannot use temp (type Text) as type string in assignment
gen-go/package/XXXX.go:3028:8: undefined: thrift.ResponseMeta
gen-go/package/XXXX.go:3053:43: undefined: thrift.ResponseMeta
gen-go/package/XXXX.go:3057:49: undefined: thrift.ResponseMeta
gen-go/package/XXXX.go:3069:14: undefined: thrift.ResponseMeta
gen-go/package/XXXX.go:3070:16: assignment mismatch: 2 variables but p.Client_().Call returns 1 values
gen-go/package/XXXX.go:3092:15: undefined: thrift.ResponseMeta
gen-go/package/XXXX.go:3093:17: assignment mismatch: 2 variables but p.Client_().Call returns 1 values
gen-go/package/XXXX.go:3114:15: undefined: thrift.ResponseMeta
gen-go/package/XXXX.go:3115:17: assignment mismatch: 2 variables but p.Client_().Call returns 1 values
gen-go/package/XXXX.go:3115:17: too many errors

It seems that when I install the library with command go get github.com/apache/thrift/lib/go/thrift/..., it installed with an old version code but not latest. The gen-codes were generated by thrift-0.14.2 which is latest version.

fishy · 2021-07-23T14:25:27Z

@litao3rd you are not using latest thrift go library, please check your go.mod file to make sure you are using the correct version of the library.

litao3rd · 2021-07-24T10:05:13Z

@fishy thanks for your reply. I'm sure I was using the latest thrift go library.

I tested under a pure new environment built by these steps.

1, download the latest go executable and installed it with the version go1.16.6
2, create a new go project by command `go mod init demo`
3, generate thrift codes by command `thrift -gen go file.thrift`
4, installed the latest go thrift library by command `go get github.com/apache/thrift/lib/go/thrift/...`
5, run command `go build`

the generated go.mod file showed as below

module demo

go 1.16

require github.com/apache/thrift/lib/go/thrift v0.0.0-20210120171102-e27e82c46ba4 // indirect

the generated go.sum file showed as below

github.com/apache/thrift/lib/go/thrift v0.0.0-20210120171102-e27e82c46ba4 h1:orNYqmQGnSjgOauLWjHEp9/qIDT98xv/0Aa4Zet3/Y8=
github.com/apache/thrift/lib/go/thrift v0.0.0-20210120171102-e27e82c46ba4/go.mod h1:V/LzksIyqd3KZuQ2SunvReTG/UkArhII1dAWY5U1sCE=

It's weird that when I install the thrift library it prompts I'm installing v0.14.2 which is the latest version. But in go.mod file there is no version information just v0.0.0.

go: downloading github.com/apache/thrift v0.14.2
go: downloading github.com/apache/thrift/lib/go/thrift v0.0.0-20210120171102-e27e82c46ba4
go get: added github.com/apache/thrift/lib/go/thrift v0.0.0-20210120171102-e27e82c46ba4

fishy · 2021-07-24T15:00:15Z

That is the wrong version. On step 4, do this instead: go get github.com/apache/thrift.

That will give you v0.14.2 instead of the wrong version.

litao3rd · 2021-07-24T15:43:38Z

Hi, thanks. I did as follow commands.

1, remove the directory $HOME/go/pkg/mod as I did not set $GOROOT and $GOPATH explicitly.
2, run the command `go clean -cache`
3, go get github.com/apache/thrift
4, go mod init demo2
5, go build

On step3, I do get the right version v0.14.2 as I get the message

go: downloading github.com/apache/thrift v0.14.2

and the directory

$HOME/go/pkg/mod/github.com/apache/[email protected]

But on step4, I get the prompt

go: creating new go.mod: module demo2
go: to add module requirements and sums:
	go mod tidy

If I do as it requires to run the command go mod tidy, I will get the follow messages.

go: finding module for package github.com/apache/thrift/lib/go/thrift
go: found github.com/apache/thrift/lib/go/thrift in github.com/apache/thrift/lib/go/thrift v0.0.0-20210120171102-e27e82c46ba4

Yep, I still get the wrong version of the library.

If I do NOT run the command go mod tidy, but run go build directly. I will get the follow messages

gen-go/${pkg_name}/${filename}-consts.go:10:2: no required module provides package github.com/apache/thrift/lib/go/thrift; to add it:
	go get github.com/apache/thrift/lib/go/thrift

That meas I must run the command go get before go build. But when I run the go get command as it prompts. I get the follow messages

go: downloading github.com/apache/thrift/lib/go/thrift v0.0.0-20210120171102-e27e82c46ba4
go get: added github.com/apache/thrift/lib/go/thrift v0.0.0-20210120171102-e27e82c46ba4

I still get the wrong version.

I do not know does it any relation with my go executable version as I use the latest version of go 1.16.6

Thanks very much.

fishy · 2021-07-24T16:11:32Z

That's because you also swapped the steps. Don't do that. You need to do go get after go mod init.

Can you go back to the exact steps of #2315 (comment), and only change Step 4 into go get github.com/apache/thrift?

litao3rd · 2021-07-24T16:40:56Z

You are right. The problem is solved as your instructions. Thanks very much.

litao3rd · 2021-07-24T16:48:17Z

By the way, I installed the go thrift library as the instruction introduced in README file. Maybe it should correct or not ? As I am not good at go.

fishy · 2021-07-24T17:36:04Z

Thanks, I'll update the readme file.

…

On Sat, Jul 24, 2021, 09:48 litao3rd ***@***.***> wrote: By the way, I installed the go thrift library as the instruction introduced in README <https://github.com/apache/thrift/blob/master/lib/go/README.md> file. Maybe it should correct ? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2315 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAATCZ6WKCLVHVKPMHHFWHTTZLVF3ANCNFSM4WOZ7FIA> .

fishy requested a review from dcelasun January 22, 2021 17:38

dcelasun reviewed Jan 22, 2021

View reviewed changes

dcelasun approved these changes Jan 22, 2021

View reviewed changes

fishy force-pushed the go-call-return-map branch from 6d91313 to 7f41583 Compare January 22, 2021 17:52

fishy commented Jan 22, 2021

View reviewed changes

fishy force-pushed the go-call-return-map branch from 7f41583 to b4096fc Compare January 22, 2021 18:52

fishy force-pushed the go-call-return-map branch from b4096fc to 36e399e Compare January 22, 2021 23:51

fishy changed the title ~~THRIFT-4914: Make TClient.Call to return the response headers~~ THRIFT-4914: Make TClient.Call to return the response meta Jan 22, 2021

fishy mentioned this pull request Jan 23, 2021

Make GetResponseHeadersFromClient work with client middlewares #2314

Closed

fishy force-pushed the go-call-return-map branch from 36e399e to 9b28a26 Compare January 23, 2021 00:39

fishy merged commit c2ddaf0 into apache:master Jan 23, 2021

fishy deleted the go-call-return-map branch January 23, 2021 04:50

yhlee-tw reviewed Feb 17, 2021

View reviewed changes

fishy added a commit to fishy/thrift that referenced this pull request Feb 17, 2021

THRIFT-4914: Fix name redeclaration bug in compiled go code

b5868ed

Client: go This fixes the bug reported in apache#2315 (comment).

fishy added a commit to fishy/thrift that referenced this pull request Feb 17, 2021

THRIFT-4914: Fix name redeclaration bug in compiled go code

d22c0d8

Client: go This fixes the bug reported in apache#2315 (comment).

fishy mentioned this pull request Feb 17, 2021

THRIFT-4914: Fix name redeclaration bug in compiled go code #2332

Merged

fishy added a commit to fishy/thrift that referenced this pull request Feb 17, 2021

THRIFT-4914: Fix name redeclaration bug in compiled go code

6d4f0a4

Client: go This fixes the bug reported in apache#2315 (comment).

fishy added a commit to fishy/thrift that referenced this pull request Feb 17, 2021

THRIFT-4914: Fix name redeclaration bug in compiled go code

9a447cd

Client: go This fixes the bug reported in apache#2315 (comment).

fishy added a commit to fishy/thrift that referenced this pull request Feb 17, 2021

THRIFT-4914: Fix name redeclaration bug in compiled go code

ba00c9a

Client: go This fixes the bug reported in apache#2315 (comment).

fishy added a commit to fishy/thrift that referenced this pull request Feb 18, 2021

THRIFT-4914: Fix name redeclaration bug in compiled go code

18d9188

Client: go This fixes the bug reported in apache#2315 (comment).

fishy added a commit that referenced this pull request Feb 18, 2021

THRIFT-4914: Fix name redeclaration bug in compiled go code

a2522c0

Client: go This fixes the bug reported in #2315 (comment).

fishy added a commit to fishy/thrift that referenced this pull request Feb 18, 2021

THRIFT-4914: Fix name redeclaration bug in compiled go code

bc15861

Client: go This fixes the bug reported in apache#2315 (comment).

fishy added a commit that referenced this pull request Feb 22, 2021

THRIFT-4914: Fix name redeclaration bug in compiled go code

abb8fa8

Client: go This fixes the bug reported in #2315 (comment).

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

fishy commented Jan 22, 2021 •

edited

Loading

dcelasun Jan 22, 2021

fishy Jan 22, 2021

fishy Jan 22, 2021

dcelasun Jan 22, 2021

fishy Jan 22, 2021

fishy commented Jan 22, 2021

yhlee-tw Feb 17, 2021

fishy Feb 17, 2021

fishy Feb 17, 2021

litao3rd commented Jul 23, 2021 •

edited

Loading

fishy commented Jul 23, 2021

litao3rd commented Jul 24, 2021

fishy commented Jul 24, 2021

litao3rd commented Jul 24, 2021

fishy commented Jul 24, 2021

litao3rd commented Jul 24, 2021

litao3rd commented Jul 24, 2021 •

edited

Loading

fishy commented Jul 24, 2021 via email

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

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

Conversation

fishy commented Jan 22, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fishy commented Jan 22, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

litao3rd commented Jul 23, 2021 • edited Loading

fishy commented Jul 23, 2021

litao3rd commented Jul 24, 2021

fishy commented Jul 24, 2021

litao3rd commented Jul 24, 2021

fishy commented Jul 24, 2021

litao3rd commented Jul 24, 2021

litao3rd commented Jul 24, 2021 • edited Loading

fishy commented Jul 24, 2021 via email

fishy commented Jan 22, 2021 •

edited

Loading

litao3rd commented Jul 23, 2021 •

edited

Loading

litao3rd commented Jul 24, 2021 •

edited

Loading