[jira] [Commented] (THRIFT-4447) Golang: Panic on p.c.Call when using deprecated initializers
[ https://issues.apache.org/jira/browse/THRIFT-4447?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16319838#comment-16319838 ] ASF GitHub Bot commented on THRIFT-4447: Github user dcelasun commented on the issue: https://github.com/apache/thrift/pull/1461 Exactly. > Golang: Panic on p.c.Call when using deprecated initializers > > > Key: THRIFT-4447 > URL: https://issues.apache.org/jira/browse/THRIFT-4447 > Project: Thrift > Issue Type: Bug > Components: Go - Compiler >Affects Versions: 0.11.0 >Reporter: John Boiles > > Latest thrift:master can cause panics when using deprecated > `New*ClientFactory` and `New*ClientProtocol` functions. This happens because > both the Client and the BaseClient have an instance of a {{thrift.TClient}}. > The deprecated methods initialize the BaseClient's TClient, but other methods > use the Client's {{TClient}}. > For example, current thrift master generates structs like this > {code} > type MyServiceClient struct { > c thrift.TClient > *MyServiceBaseClient > } > type MyServiceBaseClient struct { > c thrift.TClient > } > {code} > And also a method like this: > {code} > func NewMyServiceClientFactory(t thrift.TTransport, f > thrift.TProtocolFactory) *MyServiceClient { > return {MyServiceBaseClient: > NewMyServiceBaseClientFactory(t, f)} > } > {code} > If that method is used, later calls to service methods will panic, since > {{p.c}} is nil (the actual client was stored in {{p.BaseMyServiceClient.c}}). > {code} > func (p *MyServiceClient) DoStuff(ctx context.Context, request > *DoStuffRequest) (r *DoStuffResponse, err error) { > var _args139 DoStuffArgs > _args139.Request = request > var _result140 DoStuffResult > if err = p.c.Call(ctx, "do_stuff", &_args139, &_result140); err != nil > { // PANIC > ... > {code} > In progress fix here :https://github.com/apache/thrift/pull/1461. The fix in > this PR merely sets both instances of {{TClient}} (which is what happens in > the non-deprecated {{New*Client}} function). > This patch currently fails {{make -k check}} however, since > {{src/tutorial/tutorial.go}} tries to access a different package's version of > the BaseClient. > {code} > src/tutorial/tutorial.go:477:33: bc.c undefined (cannot refer to unexported > field or method c) > {code} > The fix for that test could possibly be to expose the BaseClient's instance > of {{c}} (by making it a capital and thus exported {{C}}), or adding an > accessor method {{C()}} or {{Client()}}. > Possibly a better fix would be to either remove these deprecated methods, or > figure out which {{TClient}} is the correct one to set. > I'm not sure which is preferred, so I'm hoping to get some feedback/input > here. -- This message was sent by Atlassian JIRA (v6.4.14#64029)
[GitHub] thrift issue #1461: THRIFT-4447: Golang: Fix panic on p.c.Call when using de...
Github user dcelasun commented on the issue: https://github.com/apache/thrift/pull/1461 Exactly. ---
[GitHub] thrift issue #1461: THRIFT-4447: Golang: Fix panic on p.c.Call when using de...
Github user johnboiles commented on the issue: https://github.com/apache/thrift/pull/1461 Good call, so then: ``` func (p *BaseClient) Client_() thrift.TClient { return p.c } ``` ? ---
[jira] [Commented] (THRIFT-4447) Golang: Panic on p.c.Call when using deprecated initializers
[ https://issues.apache.org/jira/browse/THRIFT-4447?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16319802#comment-16319802 ] ASF GitHub Bot commented on THRIFT-4447: Github user johnboiles commented on the issue: https://github.com/apache/thrift/pull/1461 Good call, so then: ``` func (p *BaseClient) Client_() thrift.TClient { return p.c } ``` ? > Golang: Panic on p.c.Call when using deprecated initializers > > > Key: THRIFT-4447 > URL: https://issues.apache.org/jira/browse/THRIFT-4447 > Project: Thrift > Issue Type: Bug > Components: Go - Compiler >Affects Versions: 0.11.0 >Reporter: John Boiles > > Latest thrift:master can cause panics when using deprecated > `New*ClientFactory` and `New*ClientProtocol` functions. This happens because > both the Client and the BaseClient have an instance of a {{thrift.TClient}}. > The deprecated methods initialize the BaseClient's TClient, but other methods > use the Client's {{TClient}}. > For example, current thrift master generates structs like this > {code} > type MyServiceClient struct { > c thrift.TClient > *MyServiceBaseClient > } > type MyServiceBaseClient struct { > c thrift.TClient > } > {code} > And also a method like this: > {code} > func NewMyServiceClientFactory(t thrift.TTransport, f > thrift.TProtocolFactory) *MyServiceClient { > return {MyServiceBaseClient: > NewMyServiceBaseClientFactory(t, f)} > } > {code} > If that method is used, later calls to service methods will panic, since > {{p.c}} is nil (the actual client was stored in {{p.BaseMyServiceClient.c}}). > {code} > func (p *MyServiceClient) DoStuff(ctx context.Context, request > *DoStuffRequest) (r *DoStuffResponse, err error) { > var _args139 DoStuffArgs > _args139.Request = request > var _result140 DoStuffResult > if err = p.c.Call(ctx, "do_stuff", &_args139, &_result140); err != nil > { // PANIC > ... > {code} > In progress fix here :https://github.com/apache/thrift/pull/1461. The fix in > this PR merely sets both instances of {{TClient}} (which is what happens in > the non-deprecated {{New*Client}} function). > This patch currently fails {{make -k check}} however, since > {{src/tutorial/tutorial.go}} tries to access a different package's version of > the BaseClient. > {code} > src/tutorial/tutorial.go:477:33: bc.c undefined (cannot refer to unexported > field or method c) > {code} > The fix for that test could possibly be to expose the BaseClient's instance > of {{c}} (by making it a capital and thus exported {{C}}), or adding an > accessor method {{C()}} or {{Client()}}. > Possibly a better fix would be to either remove these deprecated methods, or > figure out which {{TClient}} is the correct one to set. > I'm not sure which is preferred, so I'm hoping to get some feedback/input > here. -- This message was sent by Atlassian JIRA (v6.4.14#64029)
[GitHub] thrift issue #1461: THRIFT-4447: Golang: Fix panic on p.c.Call when using de...
Github user dcelasun commented on the issue: https://github.com/apache/thrift/pull/1461 Any capitalized method on the client will potentially conflict with an RPC, e.g: ```thrift service Foo { string C() } ``` will generate uncompilable code. I suggest we use something like `Client_` as the method name. It looks ugly, but there is precedent for it (conflicts with keywords etc). ---
[jira] [Commented] (THRIFT-4447) Golang: Panic on p.c.Call when using deprecated initializers
[ https://issues.apache.org/jira/browse/THRIFT-4447?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16319756#comment-16319756 ] ASF GitHub Bot commented on THRIFT-4447: Github user dcelasun commented on the issue: https://github.com/apache/thrift/pull/1461 Any capitalized method on the client will potentially conflict with an RPC, e.g: ```thrift service Foo { string C() } ``` will generate uncompilable code. I suggest we use something like `Client_` as the method name. It looks ugly, but there is precedent for it (conflicts with keywords etc). > Golang: Panic on p.c.Call when using deprecated initializers > > > Key: THRIFT-4447 > URL: https://issues.apache.org/jira/browse/THRIFT-4447 > Project: Thrift > Issue Type: Bug > Components: Go - Compiler >Affects Versions: 0.11.0 >Reporter: John Boiles > > Latest thrift:master can cause panics when using deprecated > `New*ClientFactory` and `New*ClientProtocol` functions. This happens because > both the Client and the BaseClient have an instance of a {{thrift.TClient}}. > The deprecated methods initialize the BaseClient's TClient, but other methods > use the Client's {{TClient}}. > For example, current thrift master generates structs like this > {code} > type MyServiceClient struct { > c thrift.TClient > *MyServiceBaseClient > } > type MyServiceBaseClient struct { > c thrift.TClient > } > {code} > And also a method like this: > {code} > func NewMyServiceClientFactory(t thrift.TTransport, f > thrift.TProtocolFactory) *MyServiceClient { > return {MyServiceBaseClient: > NewMyServiceBaseClientFactory(t, f)} > } > {code} > If that method is used, later calls to service methods will panic, since > {{p.c}} is nil (the actual client was stored in {{p.BaseMyServiceClient.c}}). > {code} > func (p *MyServiceClient) DoStuff(ctx context.Context, request > *DoStuffRequest) (r *DoStuffResponse, err error) { > var _args139 DoStuffArgs > _args139.Request = request > var _result140 DoStuffResult > if err = p.c.Call(ctx, "do_stuff", &_args139, &_result140); err != nil > { // PANIC > ... > {code} > In progress fix here :https://github.com/apache/thrift/pull/1461. The fix in > this PR merely sets both instances of {{TClient}} (which is what happens in > the non-deprecated {{New*Client}} function). > This patch currently fails {{make -k check}} however, since > {{src/tutorial/tutorial.go}} tries to access a different package's version of > the BaseClient. > {code} > src/tutorial/tutorial.go:477:33: bc.c undefined (cannot refer to unexported > field or method c) > {code} > The fix for that test could possibly be to expose the BaseClient's instance > of {{c}} (by making it a capital and thus exported {{C}}), or adding an > accessor method {{C()}} or {{Client()}}. > Possibly a better fix would be to either remove these deprecated methods, or > figure out which {{TClient}} is the correct one to set. > I'm not sure which is preferred, so I'm hoping to get some feedback/input > here. -- This message was sent by Atlassian JIRA (v6.4.14#64029)
[GitHub] thrift issue #1449: THRIFT-4434. Update .NET Core components, add tests for ...
Github user vgotra commented on the issue: https://github.com/apache/thrift/pull/1449 @jeking3 - updated header file to fix warnings, also reverted Autotools files to check builds ---
[jira] [Commented] (THRIFT-4434) Update .NET Core components, add tests for .Net Core library and .Net Core compiler, fix bugs and build process
[ https://issues.apache.org/jira/browse/THRIFT-4434?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16319422#comment-16319422 ] ASF GitHub Bot commented on THRIFT-4434: Github user vgotra commented on the issue: https://github.com/apache/thrift/pull/1449 @jeking3 - updated header file to fix warnings, also reverted Autotools files to check builds > Update .NET Core components, add tests for .Net Core library and .Net Core > compiler, fix bugs and build process > --- > > Key: THRIFT-4434 > URL: https://issues.apache.org/jira/browse/THRIFT-4434 > Project: Thrift > Issue Type: Improvement > Components: .NETCore - Compiler, .NETCore - Library, Build Process > Environment: Windows, Linux, MacOS >Reporter: Volodymyr Gotra >Assignee: Volodymyr Gotra >Priority: Critical > > This pull request should: > - highly improve the current version of .Net Core library and .Net Core > compiler and quality of code > - improve and simplify build process > - improve documentation related to .Net Core library and compiler > - fix found bugs (some of bugs can be clarified like major - they are related > to porting of protocols from Java version and can be present in C# library) > - add important unit tests for .Net Core library and .Net Core compiler > - add possibility to easy add unit tests for compiler for other languages -- This message was sent by Atlassian JIRA (v6.4.14#64029)
[jira] [Comment Edited] (THRIFT-4413) Publish a Maven artifact for Thrift v0.11
[ https://issues.apache.org/jira/browse/THRIFT-4413?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16319415#comment-16319415 ] Buğra Gedik edited comment on THRIFT-4413 at 1/9/18 11:41 PM: -- [~jking3]: Do you think the Thrift project has someone who can do this? was (Author: bgedik): [~jking3]: Do you think the Thrift project has someone who can actually do this? > Publish a Maven artifact for Thrift v0.11 > - > > Key: THRIFT-4413 > URL: https://issues.apache.org/jira/browse/THRIFT-4413 > Project: Thrift > Issue Type: Task > Components: Java - Library >Affects Versions: 0.11.0 >Reporter: Buğra Gedik >Assignee: Jake Farrell > Fix For: 0.11.0 > > -- This message was sent by Atlassian JIRA (v6.4.14#64029)
[jira] [Commented] (THRIFT-4413) Publish a Maven artifact for Thrift v0.11
[ https://issues.apache.org/jira/browse/THRIFT-4413?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16319415#comment-16319415 ] Buğra Gedik commented on THRIFT-4413: - [~jking3]: Do you think the Thrift project has someone who can actually do this? > Publish a Maven artifact for Thrift v0.11 > - > > Key: THRIFT-4413 > URL: https://issues.apache.org/jira/browse/THRIFT-4413 > Project: Thrift > Issue Type: Task > Components: Java - Library >Affects Versions: 0.11.0 >Reporter: Buğra Gedik >Assignee: Jake Farrell > Fix For: 0.11.0 > > -- This message was sent by Atlassian JIRA (v6.4.14#64029)
[jira] [Commented] (THRIFT-4447) Golang: Panic on p.c.Call when using deprecated initializers
[ https://issues.apache.org/jira/browse/THRIFT-4447?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16319385#comment-16319385 ] ASF GitHub Bot commented on THRIFT-4447: Github user johnboiles commented on the issue: https://github.com/apache/thrift/pull/1461 This is a lot cleaner -- I like it. We still have the same issue with tests, though, if the base service is in a separate package -- you can't access the client as `.c` on the base service since it's lowercased. Maybe we expose the client with a getter method? ``` func (p *BaseClient) C() thrift.TClient { return p.c } ``` > Golang: Panic on p.c.Call when using deprecated initializers > > > Key: THRIFT-4447 > URL: https://issues.apache.org/jira/browse/THRIFT-4447 > Project: Thrift > Issue Type: Bug > Components: Go - Compiler >Affects Versions: 0.11.0 >Reporter: John Boiles > > Latest thrift:master can cause panics when using deprecated > `New*ClientFactory` and `New*ClientProtocol` functions. This happens because > both the Client and the BaseClient have an instance of a {{thrift.TClient}}. > The deprecated methods initialize the BaseClient's TClient, but other methods > use the Client's {{TClient}}. > For example, current thrift master generates structs like this > {code} > type MyServiceClient struct { > c thrift.TClient > *MyServiceBaseClient > } > type MyServiceBaseClient struct { > c thrift.TClient > } > {code} > And also a method like this: > {code} > func NewMyServiceClientFactory(t thrift.TTransport, f > thrift.TProtocolFactory) *MyServiceClient { > return {MyServiceBaseClient: > NewMyServiceBaseClientFactory(t, f)} > } > {code} > If that method is used, later calls to service methods will panic, since > {{p.c}} is nil (the actual client was stored in {{p.BaseMyServiceClient.c}}). > {code} > func (p *MyServiceClient) DoStuff(ctx context.Context, request > *DoStuffRequest) (r *DoStuffResponse, err error) { > var _args139 DoStuffArgs > _args139.Request = request > var _result140 DoStuffResult > if err = p.c.Call(ctx, "do_stuff", &_args139, &_result140); err != nil > { // PANIC > ... > {code} > In progress fix here :https://github.com/apache/thrift/pull/1461. The fix in > this PR merely sets both instances of {{TClient}} (which is what happens in > the non-deprecated {{New*Client}} function). > This patch currently fails {{make -k check}} however, since > {{src/tutorial/tutorial.go}} tries to access a different package's version of > the BaseClient. > {code} > src/tutorial/tutorial.go:477:33: bc.c undefined (cannot refer to unexported > field or method c) > {code} > The fix for that test could possibly be to expose the BaseClient's instance > of {{c}} (by making it a capital and thus exported {{C}}), or adding an > accessor method {{C()}} or {{Client()}}. > Possibly a better fix would be to either remove these deprecated methods, or > figure out which {{TClient}} is the correct one to set. > I'm not sure which is preferred, so I'm hoping to get some feedback/input > here. -- This message was sent by Atlassian JIRA (v6.4.14#64029)
[GitHub] thrift issue #1461: THRIFT-4447: Golang: Fix panic on p.c.Call when using de...
Github user johnboiles commented on the issue: https://github.com/apache/thrift/pull/1461 This is a lot cleaner -- I like it. We still have the same issue with tests, though, if the base service is in a separate package -- you can't access the client as `.c` on the base service since it's lowercased. Maybe we expose the client with a getter method? ``` func (p *BaseClient) C() thrift.TClient { return p.c } ``` ---
[jira] [Commented] (THRIFT-4413) Publish a Maven artifact for Thrift v0.11
[ https://issues.apache.org/jira/browse/THRIFT-4413?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16319312#comment-16319312 ] Doug Cutting commented on THRIFT-4413: -- I am no longer involved in the Thrift project. All committers should be able to deploy and publish to Apache's Nexus Maven repository. If @jking is unable to get this to work perhaps he should file an issue in the Infra jira with category Nexus. Ideally Thrift's full release process would be documented. For example, Avro's release process, including publishing Maven artifacts, is documented at https://cwiki.apache.org/confluence/display/AVRO/How+To+Release. Thrift's instructions do not seem to include publishing to Maven or other repositories. https://thrift.apache.org/docs/committers/HowToRelease > Publish a Maven artifact for Thrift v0.11 > - > > Key: THRIFT-4413 > URL: https://issues.apache.org/jira/browse/THRIFT-4413 > Project: Thrift > Issue Type: Task > Components: Java - Library >Affects Versions: 0.11.0 >Reporter: Buğra Gedik >Assignee: Jake Farrell > Fix For: 0.11.0 > > -- This message was sent by Atlassian JIRA (v6.4.14#64029)
[jira] [Commented] (THRIFT-4434) Update .NET Core components, add tests for .Net Core library and .Net Core compiler, fix bugs and build process
[ https://issues.apache.org/jira/browse/THRIFT-4434?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16319258#comment-16319258 ] ASF GitHub Bot commented on THRIFT-4434: Github user vgotra commented on the issue: https://github.com/apache/thrift/pull/1449 @jeking3 - It seems that CLang doesn't recognize "-Wno-error=maybe-uninitialized". Do you know how to add some switches for different compilers ? - Error mostly because of one struct in header file (but to normally fix it it will take some time - unit tests for IDL much more important - already fixed few important bugs because of unit tetst for .Net Core generator) - easier to add suppression correctly and then fix it correctly. > Update .NET Core components, add tests for .Net Core library and .Net Core > compiler, fix bugs and build process > --- > > Key: THRIFT-4434 > URL: https://issues.apache.org/jira/browse/THRIFT-4434 > Project: Thrift > Issue Type: Improvement > Components: .NETCore - Compiler, .NETCore - Library, Build Process > Environment: Windows, Linux, MacOS >Reporter: Volodymyr Gotra >Assignee: Volodymyr Gotra >Priority: Critical > > This pull request should: > - highly improve the current version of .Net Core library and .Net Core > compiler and quality of code > - improve and simplify build process > - improve documentation related to .Net Core library and compiler > - fix found bugs (some of bugs can be clarified like major - they are related > to porting of protocols from Java version and can be present in C# library) > - add important unit tests for .Net Core library and .Net Core compiler > - add possibility to easy add unit tests for compiler for other languages -- This message was sent by Atlassian JIRA (v6.4.14#64029)
[GitHub] thrift issue #1449: THRIFT-4434. Update .NET Core components, add tests for ...
Github user vgotra commented on the issue: https://github.com/apache/thrift/pull/1449 @jeking3 - It seems that CLang doesn't recognize "-Wno-error=maybe-uninitialized". Do you know how to add some switches for different compilers ? - Error mostly because of one struct in header file (but to normally fix it it will take some time - unit tests for IDL much more important - already fixed few important bugs because of unit tetst for .Net Core generator) - easier to add suppression correctly and then fix it correctly. ---
[jira] [Commented] (THRIFT-4447) Golang: Panic on p.c.Call when using deprecated initializers
[ https://issues.apache.org/jira/browse/THRIFT-4447?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16319213#comment-16319213 ] ASF GitHub Bot commented on THRIFT-4447: Github user dcelasun commented on the issue: https://github.com/apache/thrift/pull/1461 > So basically the logic would be to only add `c: thrift.TClient` to the `*Client` struct if `extends.empty()` in the generator? Exactly. > Golang: Panic on p.c.Call when using deprecated initializers > > > Key: THRIFT-4447 > URL: https://issues.apache.org/jira/browse/THRIFT-4447 > Project: Thrift > Issue Type: Bug > Components: Go - Compiler >Affects Versions: 0.11.0 >Reporter: John Boiles > > Latest thrift:master can cause panics when using deprecated > `New*ClientFactory` and `New*ClientProtocol` functions. This happens because > both the Client and the BaseClient have an instance of a {{thrift.TClient}}. > The deprecated methods initialize the BaseClient's TClient, but other methods > use the Client's {{TClient}}. > For example, current thrift master generates structs like this > {code} > type MyServiceClient struct { > c thrift.TClient > *MyServiceBaseClient > } > type MyServiceBaseClient struct { > c thrift.TClient > } > {code} > And also a method like this: > {code} > func NewMyServiceClientFactory(t thrift.TTransport, f > thrift.TProtocolFactory) *MyServiceClient { > return {MyServiceBaseClient: > NewMyServiceBaseClientFactory(t, f)} > } > {code} > If that method is used, later calls to service methods will panic, since > {{p.c}} is nil (the actual client was stored in {{p.BaseMyServiceClient.c}}). > {code} > func (p *MyServiceClient) DoStuff(ctx context.Context, request > *DoStuffRequest) (r *DoStuffResponse, err error) { > var _args139 DoStuffArgs > _args139.Request = request > var _result140 DoStuffResult > if err = p.c.Call(ctx, "do_stuff", &_args139, &_result140); err != nil > { // PANIC > ... > {code} > In progress fix here :https://github.com/apache/thrift/pull/1461. The fix in > this PR merely sets both instances of {{TClient}} (which is what happens in > the non-deprecated {{New*Client}} function). > This patch currently fails {{make -k check}} however, since > {{src/tutorial/tutorial.go}} tries to access a different package's version of > the BaseClient. > {code} > src/tutorial/tutorial.go:477:33: bc.c undefined (cannot refer to unexported > field or method c) > {code} > The fix for that test could possibly be to expose the BaseClient's instance > of {{c}} (by making it a capital and thus exported {{C}}), or adding an > accessor method {{C()}} or {{Client()}}. > Possibly a better fix would be to either remove these deprecated methods, or > figure out which {{TClient}} is the correct one to set. > I'm not sure which is preferred, so I'm hoping to get some feedback/input > here. -- This message was sent by Atlassian JIRA (v6.4.14#64029)
[GitHub] thrift issue #1461: THRIFT-4447: Golang: Fix panic on p.c.Call when using de...
Github user dcelasun commented on the issue: https://github.com/apache/thrift/pull/1461 > So basically the logic would be to only add `c: thrift.TClient` to the `*Client` struct if `extends.empty()` in the generator? Exactly. ---
[jira] [Commented] (THRIFT-4447) Golang: Panic on p.c.Call when using deprecated initializers
[ https://issues.apache.org/jira/browse/THRIFT-4447?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16319207#comment-16319207 ] ASF GitHub Bot commented on THRIFT-4447: Github user johnboiles commented on the issue: https://github.com/apache/thrift/pull/1461 You're correct that it's a service extending another service; tbh I missed that too (I'm not the author of the `.thrift` files. I'm also generally new to thrift so still sorting out what should be generated). So basically the logic would be to only add `c: thrift.TClient` to the `*Client` struct if `extends.empty()` in the generator? I'll take a look and see how hard that looks. > Golang: Panic on p.c.Call when using deprecated initializers > > > Key: THRIFT-4447 > URL: https://issues.apache.org/jira/browse/THRIFT-4447 > Project: Thrift > Issue Type: Bug > Components: Go - Compiler >Affects Versions: 0.11.0 >Reporter: John Boiles > > Latest thrift:master can cause panics when using deprecated > `New*ClientFactory` and `New*ClientProtocol` functions. This happens because > both the Client and the BaseClient have an instance of a {{thrift.TClient}}. > The deprecated methods initialize the BaseClient's TClient, but other methods > use the Client's {{TClient}}. > For example, current thrift master generates structs like this > {code} > type MyServiceClient struct { > c thrift.TClient > *MyServiceBaseClient > } > type MyServiceBaseClient struct { > c thrift.TClient > } > {code} > And also a method like this: > {code} > func NewMyServiceClientFactory(t thrift.TTransport, f > thrift.TProtocolFactory) *MyServiceClient { > return {MyServiceBaseClient: > NewMyServiceBaseClientFactory(t, f)} > } > {code} > If that method is used, later calls to service methods will panic, since > {{p.c}} is nil (the actual client was stored in {{p.BaseMyServiceClient.c}}). > {code} > func (p *MyServiceClient) DoStuff(ctx context.Context, request > *DoStuffRequest) (r *DoStuffResponse, err error) { > var _args139 DoStuffArgs > _args139.Request = request > var _result140 DoStuffResult > if err = p.c.Call(ctx, "do_stuff", &_args139, &_result140); err != nil > { // PANIC > ... > {code} > In progress fix here :https://github.com/apache/thrift/pull/1461. The fix in > this PR merely sets both instances of {{TClient}} (which is what happens in > the non-deprecated {{New*Client}} function). > This patch currently fails {{make -k check}} however, since > {{src/tutorial/tutorial.go}} tries to access a different package's version of > the BaseClient. > {code} > src/tutorial/tutorial.go:477:33: bc.c undefined (cannot refer to unexported > field or method c) > {code} > The fix for that test could possibly be to expose the BaseClient's instance > of {{c}} (by making it a capital and thus exported {{C}}), or adding an > accessor method {{C()}} or {{Client()}}. > Possibly a better fix would be to either remove these deprecated methods, or > figure out which {{TClient}} is the correct one to set. > I'm not sure which is preferred, so I'm hoping to get some feedback/input > here. -- This message was sent by Atlassian JIRA (v6.4.14#64029)
[GitHub] thrift issue #1461: THRIFT-4447: Golang: Fix panic on p.c.Call when using de...
Github user johnboiles commented on the issue: https://github.com/apache/thrift/pull/1461 You're correct that it's a service extending another service; tbh I missed that too (I'm not the author of the `.thrift` files. I'm also generally new to thrift so still sorting out what should be generated). So basically the logic would be to only add `c: thrift.TClient` to the `*Client` struct if `extends.empty()` in the generator? I'll take a look and see how hard that looks. ---
[jira] [Commented] (THRIFT-4434) Update .NET Core components, add tests for .Net Core library and .Net Core compiler, fix bugs and build process
[ https://issues.apache.org/jira/browse/THRIFT-4434?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16319169#comment-16319169 ] ASF GitHub Bot commented on THRIFT-4434: Github user vgotra commented on the issue: https://github.com/apache/thrift/pull/1449 Updated tests, fixed some problems (added framed correct support at server side, updated tests and test known failures, etc.), reverted one line in netcore*.h file (instead added suppression of one warning to automake files). It seems, that at Ubuntu 17.10, it can build and run all tests for .net core (for server side and client side) for all pre-configured languages with automake (make cross). > Update .NET Core components, add tests for .Net Core library and .Net Core > compiler, fix bugs and build process > --- > > Key: THRIFT-4434 > URL: https://issues.apache.org/jira/browse/THRIFT-4434 > Project: Thrift > Issue Type: Improvement > Components: .NETCore - Compiler, .NETCore - Library, Build Process > Environment: Windows, Linux, MacOS >Reporter: Volodymyr Gotra >Assignee: Volodymyr Gotra >Priority: Critical > > This pull request should: > - highly improve the current version of .Net Core library and .Net Core > compiler and quality of code > - improve and simplify build process > - improve documentation related to .Net Core library and compiler > - fix found bugs (some of bugs can be clarified like major - they are related > to porting of protocols from Java version and can be present in C# library) > - add important unit tests for .Net Core library and .Net Core compiler > - add possibility to easy add unit tests for compiler for other languages -- This message was sent by Atlassian JIRA (v6.4.14#64029)
[GitHub] thrift issue #1449: THRIFT-4434. Update .NET Core components, add tests for ...
Github user vgotra commented on the issue: https://github.com/apache/thrift/pull/1449 Updated tests, fixed some problems (added framed correct support at server side, updated tests and test known failures, etc.), reverted one line in netcore*.h file (instead added suppression of one warning to automake files). It seems, that at Ubuntu 17.10, it can build and run all tests for .net core (for server side and client side) for all pre-configured languages with automake (make cross). ---
[jira] [Commented] (THRIFT-4447) Golang: Panic on p.c.Call when using deprecated initializers
[ https://issues.apache.org/jira/browse/THRIFT-4447?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16319105#comment-16319105 ] ASF GitHub Bot commented on THRIFT-4447: Github user dcelasun commented on the issue: https://github.com/apache/thrift/pull/1461 Ah, you have a service extending another service. I missed that. In that case, I think removing `c thrift.TClient` from `UserServiceClient` (and constructor functions) is the cleanest option. That way, we would only have a `TClient` within the `UserServiceBase` and it can be reused by any extending service. Thoughts? > Golang: Panic on p.c.Call when using deprecated initializers > > > Key: THRIFT-4447 > URL: https://issues.apache.org/jira/browse/THRIFT-4447 > Project: Thrift > Issue Type: Bug > Components: Go - Compiler >Affects Versions: 0.11.0 >Reporter: John Boiles > > Latest thrift:master can cause panics when using deprecated > `New*ClientFactory` and `New*ClientProtocol` functions. This happens because > both the Client and the BaseClient have an instance of a {{thrift.TClient}}. > The deprecated methods initialize the BaseClient's TClient, but other methods > use the Client's {{TClient}}. > For example, current thrift master generates structs like this > {code} > type MyServiceClient struct { > c thrift.TClient > *MyServiceBaseClient > } > type MyServiceBaseClient struct { > c thrift.TClient > } > {code} > And also a method like this: > {code} > func NewMyServiceClientFactory(t thrift.TTransport, f > thrift.TProtocolFactory) *MyServiceClient { > return {MyServiceBaseClient: > NewMyServiceBaseClientFactory(t, f)} > } > {code} > If that method is used, later calls to service methods will panic, since > {{p.c}} is nil (the actual client was stored in {{p.BaseMyServiceClient.c}}). > {code} > func (p *MyServiceClient) DoStuff(ctx context.Context, request > *DoStuffRequest) (r *DoStuffResponse, err error) { > var _args139 DoStuffArgs > _args139.Request = request > var _result140 DoStuffResult > if err = p.c.Call(ctx, "do_stuff", &_args139, &_result140); err != nil > { // PANIC > ... > {code} > In progress fix here :https://github.com/apache/thrift/pull/1461. The fix in > this PR merely sets both instances of {{TClient}} (which is what happens in > the non-deprecated {{New*Client}} function). > This patch currently fails {{make -k check}} however, since > {{src/tutorial/tutorial.go}} tries to access a different package's version of > the BaseClient. > {code} > src/tutorial/tutorial.go:477:33: bc.c undefined (cannot refer to unexported > field or method c) > {code} > The fix for that test could possibly be to expose the BaseClient's instance > of {{c}} (by making it a capital and thus exported {{C}}), or adding an > accessor method {{C()}} or {{Client()}}. > Possibly a better fix would be to either remove these deprecated methods, or > figure out which {{TClient}} is the correct one to set. > I'm not sure which is preferred, so I'm hoping to get some feedback/input > here. -- This message was sent by Atlassian JIRA (v6.4.14#64029)
[GitHub] thrift issue #1461: THRIFT-4447: Golang: Fix panic on p.c.Call when using de...
Github user dcelasun commented on the issue: https://github.com/apache/thrift/pull/1461 Ah, you have a service extending another service. I missed that. In that case, I think removing `c thrift.TClient` from `UserServiceClient` (and constructor functions) is the cleanest option. That way, we would only have a `TClient` within the `UserServiceBase` and it can be reused by any extending service. Thoughts? ---
[jira] [Commented] (THRIFT-4447) Golang: Panic on p.c.Call when using deprecated initializers
[ https://issues.apache.org/jira/browse/THRIFT-4447?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16319090#comment-16319090 ] ASF GitHub Bot commented on THRIFT-4447: Github user johnboiles commented on the issue: https://github.com/apache/thrift/pull/1461 > Are you getting this second "Base" struct with 0.11 compiler? Because I can't reproduce. I've been working off the master branch, but I just tried generation with 0.11 (327ebb6c2b6df8bf075da02ef45a2a034e9b79ba) and it looks like it has the same issue. Here is a direct copy-paste from my generated code with 0.11: ``` // Deprecated: Use NewUserService instead func NewUserServiceClientFactory(t thrift.TTransport, f thrift.TProtocolFactory) *UserServiceClient { return {UserServiceBaseClient: NewUserServiceBaseClientFactory(t, f)} } // Deprecated: Use NewUserService instead func NewUserServiceClientProtocol(t thrift.TTransport, iprot thrift.TProtocol, oprot thrift.TProtocol) *UserServiceClient { return {UserServiceBaseClient: NewUserServiceBaseClientProtocol(t, iprot, oprot)} } func NewUserServiceClient(c thrift.TClient) *UserServiceClient { return { c: c, UserServiceBaseClient: NewUserServiceBaseClient(c), } } ``` > Golang: Panic on p.c.Call when using deprecated initializers > > > Key: THRIFT-4447 > URL: https://issues.apache.org/jira/browse/THRIFT-4447 > Project: Thrift > Issue Type: Bug > Components: Go - Compiler >Affects Versions: 0.11.0 >Reporter: John Boiles > > Latest thrift:master can cause panics when using deprecated > `New*ClientFactory` and `New*ClientProtocol` functions. This happens because > both the Client and the BaseClient have an instance of a {{thrift.TClient}}. > The deprecated methods initialize the BaseClient's TClient, but other methods > use the Client's {{TClient}}. > For example, current thrift master generates structs like this > {code} > type MyServiceClient struct { > c thrift.TClient > *MyServiceBaseClient > } > type MyServiceBaseClient struct { > c thrift.TClient > } > {code} > And also a method like this: > {code} > func NewMyServiceClientFactory(t thrift.TTransport, f > thrift.TProtocolFactory) *MyServiceClient { > return {MyServiceBaseClient: > NewMyServiceBaseClientFactory(t, f)} > } > {code} > If that method is used, later calls to service methods will panic, since > {{p.c}} is nil (the actual client was stored in {{p.BaseMyServiceClient.c}}). > {code} > func (p *MyServiceClient) DoStuff(ctx context.Context, request > *DoStuffRequest) (r *DoStuffResponse, err error) { > var _args139 DoStuffArgs > _args139.Request = request > var _result140 DoStuffResult > if err = p.c.Call(ctx, "do_stuff", &_args139, &_result140); err != nil > { // PANIC > ... > {code} > In progress fix here :https://github.com/apache/thrift/pull/1461. The fix in > this PR merely sets both instances of {{TClient}} (which is what happens in > the non-deprecated {{New*Client}} function). > This patch currently fails {{make -k check}} however, since > {{src/tutorial/tutorial.go}} tries to access a different package's version of > the BaseClient. > {code} > src/tutorial/tutorial.go:477:33: bc.c undefined (cannot refer to unexported > field or method c) > {code} > The fix for that test could possibly be to expose the BaseClient's instance > of {{c}} (by making it a capital and thus exported {{C}}), or adding an > accessor method {{C()}} or {{Client()}}. > Possibly a better fix would be to either remove these deprecated methods, or > figure out which {{TClient}} is the correct one to set. > I'm not sure which is preferred, so I'm hoping to get some feedback/input > here. -- This message was sent by Atlassian JIRA (v6.4.14#64029)
[GitHub] thrift issue #1461: THRIFT-4447: Golang: Fix panic on p.c.Call when using de...
Github user johnboiles commented on the issue: https://github.com/apache/thrift/pull/1461 > Are you getting this second "Base" struct with 0.11 compiler? Because I can't reproduce. I've been working off the master branch, but I just tried generation with 0.11 (327ebb6c2b6df8bf075da02ef45a2a034e9b79ba) and it looks like it has the same issue. Here is a direct copy-paste from my generated code with 0.11: ``` // Deprecated: Use NewUserService instead func NewUserServiceClientFactory(t thrift.TTransport, f thrift.TProtocolFactory) *UserServiceClient { return {UserServiceBaseClient: NewUserServiceBaseClientFactory(t, f)} } // Deprecated: Use NewUserService instead func NewUserServiceClientProtocol(t thrift.TTransport, iprot thrift.TProtocol, oprot thrift.TProtocol) *UserServiceClient { return {UserServiceBaseClient: NewUserServiceBaseClientProtocol(t, iprot, oprot)} } func NewUserServiceClient(c thrift.TClient) *UserServiceClient { return { c: c, UserServiceBaseClient: NewUserServiceBaseClient(c), } } ``` ---
[jira] [Comment Edited] (THRIFT-4413) Publish a Maven artifact for Thrift v0.11
[ https://issues.apache.org/jira/browse/THRIFT-4413?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16319071#comment-16319071 ] Buğra Gedik edited comment on THRIFT-4413 at 1/9/18 8:19 PM: - There are no comments from [~jfarrell] or anyone else and this is pretty much stuck. Does anyone know what action can be taken? [~cutting]: I saw your name in this page: https://thrift.apache.org/about as the project champion and as such, I am mentioning you here. We are unable to get Thrift 0.11.0 into Maven and PyPi, despite reporting the issue here and in THRIFT-4414. It seems the people who have the authority to do this are not responding to our requests. The same ignorance was shown here: https://issues.apache.org/jira/browse/THRIFT-4250. If you read that thread, you can see how the release was handled improperly ([~ctubbsii]'s comment [here|https://issues.apache.org/jira/browse/THRIFT-4250?focusedCommentId=16291576=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-16291576] summarizes it really well). was (Author: bgedik): There are no comments from [~jfarrell] or anyone else and this is pretty much stuck. Does anyone know what action can be taken? [~cutting]: I saw your name in this page: https://thrift.apache.org/about as the project champion and as such, I am mentioning you here. We are unable to get Thrift 0.11.0 in Maven and PyPi, despite reporting the issue here and in THRIFT-4414. It seems the people who have the authority to do this are not responding to our requests. The same ignorance was shown here: https://issues.apache.org/jira/browse/THRIFT-4250. If you read that thread, you can see how the release was handled improperly ([~ctubbsii]'s comment [here|https://issues.apache.org/jira/browse/THRIFT-4250?focusedCommentId=16291576=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-16291576] summarizes it really well). > Publish a Maven artifact for Thrift v0.11 > - > > Key: THRIFT-4413 > URL: https://issues.apache.org/jira/browse/THRIFT-4413 > Project: Thrift > Issue Type: Task > Components: Java - Library >Affects Versions: 0.11.0 >Reporter: Buğra Gedik >Assignee: Jake Farrell > Fix For: 0.11.0 > > -- This message was sent by Atlassian JIRA (v6.4.14#64029)
[jira] [Comment Edited] (THRIFT-4413) Publish a Maven artifact for Thrift v0.11
[ https://issues.apache.org/jira/browse/THRIFT-4413?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16319071#comment-16319071 ] Buğra Gedik edited comment on THRIFT-4413 at 1/9/18 8:17 PM: - There are no comments from [~jfarrell] or anyone else and this is pretty much stuck. Does anyone know what action can be taken? [~cutting]: I saw your name in this page: https://thrift.apache.org/about as the project champion and as such, I am mentioning you here. We are unable to get Thrift 0.11.0 in Maven and PyPi, despite reporting the issue here and in THRIFT-4414. It seems the people who have the authority to do this are not responding to our requests. The same ignorance was shown here: https://issues.apache.org/jira/browse/THRIFT-4250. If you read that thread, you can see how the release was handled improperly ([~ctubbsii]'s comment [here|https://issues.apache.org/jira/browse/THRIFT-4250?focusedCommentId=16291576=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-16291576] summarizes it really well). was (Author: bgedik): There are no comments from [~jfarrell] or anyone else and this is pretty much stuck. Does anyone know what action can be taken? [~cutting]: I saw your name in this page: https://thrift.apache.org/about as the project champion and as such, mentioning you here. We are unable to get Thrift 0.11.0 in maven and PyPi, despite reporting the issue here and in THRIFT-4414. It seems the people who have the authority to do this are not responding to our requests. The same ignorance was shown here: https://issues.apache.org/jira/browse/THRIFT-4250. If you read that thread, you can see how the release was handled improperly ([~ctubbsii]'s comment [here|https://issues.apache.org/jira/browse/THRIFT-4250?focusedCommentId=16291576=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-16291576] summarizes it really well). > Publish a Maven artifact for Thrift v0.11 > - > > Key: THRIFT-4413 > URL: https://issues.apache.org/jira/browse/THRIFT-4413 > Project: Thrift > Issue Type: Task > Components: Java - Library >Affects Versions: 0.11.0 >Reporter: Buğra Gedik >Assignee: Jake Farrell > Fix For: 0.11.0 > > -- This message was sent by Atlassian JIRA (v6.4.14#64029)
[jira] [Comment Edited] (THRIFT-4413) Publish a Maven artifact for Thrift v0.11
[ https://issues.apache.org/jira/browse/THRIFT-4413?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16319071#comment-16319071 ] Buğra Gedik edited comment on THRIFT-4413 at 1/9/18 8:16 PM: - There are no comments from [~jfarrell] or anyone else and this is pretty much stuck. Does anyone know what action can be taken? [~cutting]: I saw your name in this page: https://thrift.apache.org/about as the project champion and as such, mentioning you here. We are unable to get Thrift 0.11.0 in maven and PyPi, despite reporting the issue here and in THRIFT-4414. It seems the people who have the authority to do this are not responding to our requests. The same ignorance was shown here: https://issues.apache.org/jira/browse/THRIFT-4250. If you read that thread, you can see how the release was handled improperly ([~ctubbsii]'s comment [here|https://issues.apache.org/jira/browse/THRIFT-4250?focusedCommentId=16291576=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-16291576] summarizes it really well). was (Author: bgedik): There are no comment from [~jfarrell] or anyone else and this is pretty much stuck. Does anyone know what action can be taken? [~cutting]: I saw your name in this page: https://thrift.apache.org/about as the project champion and as such, mentioning you here. We are unable to get Thrift 0.11.0 in maven and PyPi, despite reporting the issue here and in THRIFT-4414. It seems the people who have the authority to do this are not responding to our requests. The same ignorance was shown here: https://issues.apache.org/jira/browse/THRIFT-4250. If you read that thread, you can see how the release was handled improperly ([~ctubbsii]'s comment [here|https://issues.apache.org/jira/browse/THRIFT-4250?focusedCommentId=16291576=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-16291576] summarizes it really well). > Publish a Maven artifact for Thrift v0.11 > - > > Key: THRIFT-4413 > URL: https://issues.apache.org/jira/browse/THRIFT-4413 > Project: Thrift > Issue Type: Task > Components: Java - Library >Affects Versions: 0.11.0 >Reporter: Buğra Gedik >Assignee: Jake Farrell > Fix For: 0.11.0 > > -- This message was sent by Atlassian JIRA (v6.4.14#64029)
[jira] [Comment Edited] (THRIFT-4413) Publish a Maven artifact for Thrift v0.11
[ https://issues.apache.org/jira/browse/THRIFT-4413?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16319071#comment-16319071 ] Buğra Gedik edited comment on THRIFT-4413 at 1/9/18 8:16 PM: - There are no comment from [~jfarrell] or anyone else and this is pretty much stuck. Does anyone know what action can be taken? [~cutting]: I saw your name in this page: https://thrift.apache.org/about as the project champion and as such, mentioning you here. We are unable to get Thrift 0.11.0 in maven and PyPi, despite reporting the issue here and in THRIFT-4414. It seems the people who have the authority to do this are not responding to our requests. The same ignorance was shown here: https://issues.apache.org/jira/browse/THRIFT-4250. If you read that thread, you can see how the release was handled improperly ([~ctubbsii]'s comment [here|https://issues.apache.org/jira/browse/THRIFT-4250?focusedCommentId=16291576=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-16291576] summarizes it really well). was (Author: bgedik): There no comment from [~jfarrell] or anyone else and this is pretty much stuck. Does anyone know what action can be taken? [~cutting]: I saw your name in this page: https://thrift.apache.org/about as the project champion and as such, mentioning you here. We are unable to get Thrift 0.11.0 in maven and PyPi, despite reporting the issue here and in THRIFT-4414. It seems the people who have the authority to do this are not responding to our requests. The same ignorance was shown here: https://issues.apache.org/jira/browse/THRIFT-4250. If you read that thread, you can see how the release was handled improperly ([~ctubbsii]'s comment [here|https://issues.apache.org/jira/browse/THRIFT-4250?focusedCommentId=16291576=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-16291576] summarizes it really well). > Publish a Maven artifact for Thrift v0.11 > - > > Key: THRIFT-4413 > URL: https://issues.apache.org/jira/browse/THRIFT-4413 > Project: Thrift > Issue Type: Task > Components: Java - Library >Affects Versions: 0.11.0 >Reporter: Buğra Gedik >Assignee: Jake Farrell > Fix For: 0.11.0 > > -- This message was sent by Atlassian JIRA (v6.4.14#64029)
[jira] [Comment Edited] (THRIFT-4413) Publish a Maven artifact for Thrift v0.11
[ https://issues.apache.org/jira/browse/THRIFT-4413?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16319071#comment-16319071 ] Buğra Gedik edited comment on THRIFT-4413 at 1/9/18 8:16 PM: - There no comment from [~jfarrell] or anyone else and this is pretty much stuck. Does anyone know what action can be taken? [~cutting]: I saw your name in this page: https://thrift.apache.org/about as the project champion and as such, mentioning you here. We are unable to get Thrift 0.11.0 in maven and PyPi, despite reporting the issue here and in THRIFT-4414. It seems the people who have the authority to do this are not responding to our requests. The same ignorance was shown here: https://issues.apache.org/jira/browse/THRIFT-4250. If you read that thread, you can see how the release was handled improperly ([~ctubbsii]'s comment [here|https://issues.apache.org/jira/browse/THRIFT-4250?focusedCommentId=16291576=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-16291576] summarizes it really well). was (Author: bgedik): There no comment from [~jfarrell] or anyone else and this is pretty much stuck. Does anyone know what action can be taken? [~cutting]: I saw your name in this page: https://thrift.apache.org/about as the project champion and as such, mentioning you here. We are unable to get Thrift 0.11.0 in maven and PyPi, despite reporting the issue here and in THRIFT-4414. It seems the people who have the authority to do this are not responding to our requests. The same ignorance was shown here: https://issues.apache.org/jira/browse/THRIFT-4250. If you read that thread, you can see how the release was handled improperly ([~ctubbsii]'s comment [https://issues.apache.org/jira/browse/THRIFT-4250?focusedCommentId=16291576=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-16291576|here] summarizes it really well). > Publish a Maven artifact for Thrift v0.11 > - > > Key: THRIFT-4413 > URL: https://issues.apache.org/jira/browse/THRIFT-4413 > Project: Thrift > Issue Type: Task > Components: Java - Library >Affects Versions: 0.11.0 >Reporter: Buğra Gedik >Assignee: Jake Farrell > Fix For: 0.11.0 > > -- This message was sent by Atlassian JIRA (v6.4.14#64029)
[jira] [Commented] (THRIFT-4413) Publish a Maven artifact for Thrift v0.11
[ https://issues.apache.org/jira/browse/THRIFT-4413?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16319071#comment-16319071 ] Buğra Gedik commented on THRIFT-4413: - There no comment from [~jfarrell] or anyone else and this is pretty much stuck. Does anyone know what action can be taken? [~cutting]: I saw your name in this page: https://thrift.apache.org/about as the project champion and as such, mentioning you here. We are unable to get Thrift 0.11.0 in maven and PyPi, despite reporting the issue here and in THRIFT-4414. It seems the people who have the authority to do this are not responding to our requests. The same ignorance was shown here: https://issues.apache.org/jira/browse/THRIFT-4250. If you read that thread, you can see how the release was handled improperly ([~ctubbsii]'s comment [https://issues.apache.org/jira/browse/THRIFT-4250?focusedCommentId=16291576=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-16291576|here] summarizes it really well). > Publish a Maven artifact for Thrift v0.11 > - > > Key: THRIFT-4413 > URL: https://issues.apache.org/jira/browse/THRIFT-4413 > Project: Thrift > Issue Type: Task > Components: Java - Library >Affects Versions: 0.11.0 >Reporter: Buğra Gedik >Assignee: Jake Farrell > Fix For: 0.11.0 > > -- This message was sent by Atlassian JIRA (v6.4.14#64029)
[jira] [Commented] (THRIFT-4448) Golang: do something with context.Context
[ https://issues.apache.org/jira/browse/THRIFT-4448?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16319058#comment-16319058 ] ASF GitHub Bot commented on THRIFT-4448: Github user johnboiles commented on the issue: https://github.com/apache/thrift/pull/1459 Thank you for the explainer @dcelasun! Makes sense to me. @Jens-G, @jeking3: if one of you sign off on this, I'll make the changes to drop go1.6 and `x/net/context` support from thrift:master. > Golang: do something with context.Context > - > > Key: THRIFT-4448 > URL: https://issues.apache.org/jira/browse/THRIFT-4448 > Project: Thrift > Issue Type: Task > Components: Go - Library >Affects Versions: 0.11.0 >Reporter: John Boiles > > PR Here: https://github.com/apache/thrift/pull/1459 > This patch wires through {{context.Context}} such that it can be used in in > {{http.Request}}'s {{WithContext}} method. This allows Thrift HTTP requests > to canceled or timed out via the context. > This patch breaks support for go<1.7 so it's not ready to ship, but I'm > hoping to get some direction on this. When does Thrift expect to drop support > of go1.7? It looks like the current solution is to duplicate files that need > to use {{golang.org/x/net/context}} and add a {{// +build !go1.7}} but > duplication seems unsustainable as the {{context}} package is imported more > places. > Go 1.7 was released 15 August 2016. Given Golang has had significant > performance improvements in most dot releases, I suspect most production > services stay reasonably up to date. Here at Periscope/Twitter we're on > go1.9.1, and we're a fairly large organization. -- This message was sent by Atlassian JIRA (v6.4.14#64029)
[GitHub] thrift issue #1459: THRIFT-4448: Golang: do something with context.Context
Github user johnboiles commented on the issue: https://github.com/apache/thrift/pull/1459 Thank you for the explainer @dcelasun! Makes sense to me. @Jens-G, @jeking3: if one of you sign off on this, I'll make the changes to drop go1.6 and `x/net/context` support from thrift:master. ---
[jira] [Commented] (THRIFT-4448) Golang: do something with context.Context
[ https://issues.apache.org/jira/browse/THRIFT-4448?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16318965#comment-16318965 ] ASF GitHub Bot commented on THRIFT-4448: Github user dcelasun commented on the issue: https://github.com/apache/thrift/pull/1459 > I personally think dropping go1.6/x/net/context support is best for the project. Are you the right person to make that call? Sounds good to me, but we need to run it by someone with commit bits. cc @Jens-G @jeking3 > Related question: why was 'context support' added when it doesn't seem to be hooked up to anything in the library? Seems like the method signatures were the only thing that changed. It was added to support passing arbitrary values into RPC handlers. One of the big use cases was accessing client IP from an RPC handler. Before `context`, this was impossible since the underlying transport was not visible to the handler. Now, you can implement a custom `TProcessor` that embeds the generated processor, and pass the IP manually, something like: ```go type MyProcessor struct { *GeneratedProcessor } func (p *MyProcessor) Process(ctx context.Context, in, out TProtocol) (bool, TException) { name, _, seqId, err := iprot.ReadMessageBegin() if err != nil { return false, err } if processor, ok := p.GetProcessorFunction(name); ok { //TODO: get IP from transport and add to ctx return processor.Process(ctx, seqId, iprot, oprot) } // rest of it... } } ``` > Golang: do something with context.Context > - > > Key: THRIFT-4448 > URL: https://issues.apache.org/jira/browse/THRIFT-4448 > Project: Thrift > Issue Type: Task > Components: Go - Library >Affects Versions: 0.11.0 >Reporter: John Boiles > > PR Here: https://github.com/apache/thrift/pull/1459 > This patch wires through {{context.Context}} such that it can be used in in > {{http.Request}}'s {{WithContext}} method. This allows Thrift HTTP requests > to canceled or timed out via the context. > This patch breaks support for go<1.7 so it's not ready to ship, but I'm > hoping to get some direction on this. When does Thrift expect to drop support > of go1.7? It looks like the current solution is to duplicate files that need > to use {{golang.org/x/net/context}} and add a {{// +build !go1.7}} but > duplication seems unsustainable as the {{context}} package is imported more > places. > Go 1.7 was released 15 August 2016. Given Golang has had significant > performance improvements in most dot releases, I suspect most production > services stay reasonably up to date. Here at Periscope/Twitter we're on > go1.9.1, and we're a fairly large organization. -- This message was sent by Atlassian JIRA (v6.4.14#64029)
[GitHub] thrift issue #1459: THRIFT-4448: Golang: do something with context.Context
Github user dcelasun commented on the issue: https://github.com/apache/thrift/pull/1459 > I personally think dropping go1.6/x/net/context support is best for the project. Are you the right person to make that call? Sounds good to me, but we need to run it by someone with commit bits. cc @Jens-G @jeking3 > Related question: why was 'context support' added when it doesn't seem to be hooked up to anything in the library? Seems like the method signatures were the only thing that changed. It was added to support passing arbitrary values into RPC handlers. One of the big use cases was accessing client IP from an RPC handler. Before `context`, this was impossible since the underlying transport was not visible to the handler. Now, you can implement a custom `TProcessor` that embeds the generated processor, and pass the IP manually, something like: ```go type MyProcessor struct { *GeneratedProcessor } func (p *MyProcessor) Process(ctx context.Context, in, out TProtocol) (bool, TException) { name, _, seqId, err := iprot.ReadMessageBegin() if err != nil { return false, err } if processor, ok := p.GetProcessorFunction(name); ok { //TODO: get IP from transport and add to ctx return processor.Process(ctx, seqId, iprot, oprot) } // rest of it... } } ``` ---
[GitHub] thrift issue #1459: THRIFT-4448: Golang: do something with context.Context
Github user johnboiles commented on the issue: https://github.com/apache/thrift/pull/1459 I really appreciate your feedback & involvement @dcelasun! I personally think dropping `go1.6`/`x/net/context` support is best for the project. Are you the right person to make that call? Or are there others we should check with before I put in the work. Related question: why _was_ 'context support' added when it doesn't seem to be hooked up to anything in the library? Seems like the method signatures were the only thing that changed. ---
[jira] [Commented] (THRIFT-4448) Golang: do something with context.Context
[ https://issues.apache.org/jira/browse/THRIFT-4448?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16318923#comment-16318923 ] ASF GitHub Bot commented on THRIFT-4448: Github user johnboiles commented on the issue: https://github.com/apache/thrift/pull/1459 I really appreciate your feedback & involvement @dcelasun! I personally think dropping `go1.6`/`x/net/context` support is best for the project. Are you the right person to make that call? Or are there others we should check with before I put in the work. Related question: why _was_ 'context support' added when it doesn't seem to be hooked up to anything in the library? Seems like the method signatures were the only thing that changed. > Golang: do something with context.Context > - > > Key: THRIFT-4448 > URL: https://issues.apache.org/jira/browse/THRIFT-4448 > Project: Thrift > Issue Type: Task > Components: Go - Library >Affects Versions: 0.11.0 >Reporter: John Boiles > > PR Here: https://github.com/apache/thrift/pull/1459 > This patch wires through {{context.Context}} such that it can be used in in > {{http.Request}}'s {{WithContext}} method. This allows Thrift HTTP requests > to canceled or timed out via the context. > This patch breaks support for go<1.7 so it's not ready to ship, but I'm > hoping to get some direction on this. When does Thrift expect to drop support > of go1.7? It looks like the current solution is to duplicate files that need > to use {{golang.org/x/net/context}} and add a {{// +build !go1.7}} but > duplication seems unsustainable as the {{context}} package is imported more > places. > Go 1.7 was released 15 August 2016. Given Golang has had significant > performance improvements in most dot releases, I suspect most production > services stay reasonably up to date. Here at Periscope/Twitter we're on > go1.9.1, and we're a fairly large organization. -- This message was sent by Atlassian JIRA (v6.4.14#64029)
[jira] [Commented] (THRIFT-4390) Rust binary protocol and buffered transport cannot handle writes above 4096 bytes
[ https://issues.apache.org/jira/browse/THRIFT-4390?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16318854#comment-16318854 ] ASF GitHub Bot commented on THRIFT-4390: Github user allengeorge commented on the issue: https://github.com/apache/thrift/pull/1458 @jeking3 This seems like a separate problem unfortunately. I'd removed three tests from the "known failures list" and it appears there's a specific problem related to multiplexed processors and c_glib/perl. That was an oversight :/ I'd suggest the following course of action: 1. I re-add those three test failures to the "known failures" list 2. I fix up the framed test failures with > 4k long messages; I have a patch for this already 3. I add a JIRA for the multiplexed failures and fix that in a separate PR Let me know if this is acceptable to you, and I'll go about doing that. > Rust binary protocol and buffered transport cannot handle writes above 4096 > bytes > - > > Key: THRIFT-4390 > URL: https://issues.apache.org/jira/browse/THRIFT-4390 > Project: Thrift > Issue Type: Bug > Components: Rust - Library >Affects Versions: 0.10.0 > Environment: docker image ubuntu-artful >Reporter: James E. King, III >Assignee: Allen George >Priority: Critical > > While working on improving test coverage and fixing busted cross tests I > reworked the cpp test client to send binary in at size 0, 1, 2, 4, 6, 16, > ..., 131072 and after 4096 the rust server gave up. > {noformat} > 12, 113, 114, 115, 116, 117, 118, 119, 120, 121, 122, 123, 124, 125, 126, > 127, 128]) > WARN:thrift::server::threaded: processor completed with error: TransportError > { kind: Unknown, message: "failed to write whole buffer" } > Server process is successfully killed. > {noformat} > @gadLinux this may be the root cause of some of the issues you were seeing > with the interop against c_glib recently. It is the root cause of some (if > not all of) the rs-csharp test failures. -- This message was sent by Atlassian JIRA (v6.4.14#64029)
[GitHub] thrift issue #1458: THRIFT-4390: Fix bug where binary/buffered messages > 4K...
Github user allengeorge commented on the issue: https://github.com/apache/thrift/pull/1458 @jeking3 This seems like a separate problem unfortunately. I'd removed three tests from the "known failures list" and it appears there's a specific problem related to multiplexed processors and c_glib/perl. That was an oversight :/ I'd suggest the following course of action: 1. I re-add those three test failures to the "known failures" list 2. I fix up the framed test failures with > 4k long messages; I have a patch for this already 3. I add a JIRA for the multiplexed failures and fix that in a separate PR Let me know if this is acceptable to you, and I'll go about doing that. ---
[jira] [Commented] (THRIFT-4448) Golang: do something with context.Context
[ https://issues.apache.org/jira/browse/THRIFT-4448?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16318849#comment-16318849 ] ASF GitHub Bot commented on THRIFT-4448: Github user dcelasun commented on the issue: https://github.com/apache/thrift/pull/1459 When context support was added, significant effort went into keeping 1.7 compatibility so I think it would be great if we could maintain that support. On the other hand, this patch can go into 0.12 at the earliest and by that time 1.7 will be ancient. I haven't reviewed the patch yet, but if you'd like to drop compatibility, please do so across the codebase (so no more `x/net/context` anywhere). > Golang: do something with context.Context > - > > Key: THRIFT-4448 > URL: https://issues.apache.org/jira/browse/THRIFT-4448 > Project: Thrift > Issue Type: Task > Components: Go - Library >Affects Versions: 0.11.0 >Reporter: John Boiles > > PR Here: https://github.com/apache/thrift/pull/1459 > This patch wires through {{context.Context}} such that it can be used in in > {{http.Request}}'s {{WithContext}} method. This allows Thrift HTTP requests > to canceled or timed out via the context. > This patch breaks support for go<1.7 so it's not ready to ship, but I'm > hoping to get some direction on this. When does Thrift expect to drop support > of go1.7? It looks like the current solution is to duplicate files that need > to use {{golang.org/x/net/context}} and add a {{// +build !go1.7}} but > duplication seems unsustainable as the {{context}} package is imported more > places. > Go 1.7 was released 15 August 2016. Given Golang has had significant > performance improvements in most dot releases, I suspect most production > services stay reasonably up to date. Here at Periscope/Twitter we're on > go1.9.1, and we're a fairly large organization. -- This message was sent by Atlassian JIRA (v6.4.14#64029)
[GitHub] thrift issue #1459: THRIFT-4448: Golang: do something with context.Context
Github user dcelasun commented on the issue: https://github.com/apache/thrift/pull/1459 When context support was added, significant effort went into keeping 1.7 compatibility so I think it would be great if we could maintain that support. On the other hand, this patch can go into 0.12 at the earliest and by that time 1.7 will be ancient. I haven't reviewed the patch yet, but if you'd like to drop compatibility, please do so across the codebase (so no more `x/net/context` anywhere). ---
[jira] [Commented] (THRIFT-4447) Golang: Panic on p.c.Call when using deprecated initializers
[ https://issues.apache.org/jira/browse/THRIFT-4447?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16318841#comment-16318841 ] ASF GitHub Bot commented on THRIFT-4447: Github user dcelasun commented on the issue: https://github.com/apache/thrift/pull/1461 ```go type MyServiceClient struct { c thrift.TClient *MyServiceBaseClient } type MyServiceBaseClient struct { c thrift.TClient } ``` @johnboiles Are you getting this second "Base" struct with 0.11 compiler? Because I can't reproduce. > Also should I remove the // Deprecated comments if these are not actually deprecated? Yes please. > Golang: Panic on p.c.Call when using deprecated initializers > > > Key: THRIFT-4447 > URL: https://issues.apache.org/jira/browse/THRIFT-4447 > Project: Thrift > Issue Type: Bug > Components: Go - Compiler >Affects Versions: 0.11.0 >Reporter: John Boiles > > Latest thrift:master can cause panics when using deprecated > `New*ClientFactory` and `New*ClientProtocol` functions. This happens because > both the Client and the BaseClient have an instance of a {{thrift.TClient}}. > The deprecated methods initialize the BaseClient's TClient, but other methods > use the Client's {{TClient}}. > For example, current thrift master generates structs like this > {code} > type MyServiceClient struct { > c thrift.TClient > *MyServiceBaseClient > } > type MyServiceBaseClient struct { > c thrift.TClient > } > {code} > And also a method like this: > {code} > func NewMyServiceClientFactory(t thrift.TTransport, f > thrift.TProtocolFactory) *MyServiceClient { > return {MyServiceBaseClient: > NewMyServiceBaseClientFactory(t, f)} > } > {code} > If that method is used, later calls to service methods will panic, since > {{p.c}} is nil (the actual client was stored in {{p.BaseMyServiceClient.c}}). > {code} > func (p *MyServiceClient) DoStuff(ctx context.Context, request > *DoStuffRequest) (r *DoStuffResponse, err error) { > var _args139 DoStuffArgs > _args139.Request = request > var _result140 DoStuffResult > if err = p.c.Call(ctx, "do_stuff", &_args139, &_result140); err != nil > { // PANIC > ... > {code} > In progress fix here :https://github.com/apache/thrift/pull/1461. The fix in > this PR merely sets both instances of {{TClient}} (which is what happens in > the non-deprecated {{New*Client}} function). > This patch currently fails {{make -k check}} however, since > {{src/tutorial/tutorial.go}} tries to access a different package's version of > the BaseClient. > {code} > src/tutorial/tutorial.go:477:33: bc.c undefined (cannot refer to unexported > field or method c) > {code} > The fix for that test could possibly be to expose the BaseClient's instance > of {{c}} (by making it a capital and thus exported {{C}}), or adding an > accessor method {{C()}} or {{Client()}}. > Possibly a better fix would be to either remove these deprecated methods, or > figure out which {{TClient}} is the correct one to set. > I'm not sure which is preferred, so I'm hoping to get some feedback/input > here. -- This message was sent by Atlassian JIRA (v6.4.14#64029)
[GitHub] thrift issue #1461: THRIFT-4447: Golang: Fix panic on p.c.Call when using de...
Github user dcelasun commented on the issue: https://github.com/apache/thrift/pull/1461 ```go type MyServiceClient struct { c thrift.TClient *MyServiceBaseClient } type MyServiceBaseClient struct { c thrift.TClient } ``` @johnboiles Are you getting this second "Base" struct with 0.11 compiler? Because I can't reproduce. > Also should I remove the // Deprecated comments if these are not actually deprecated? Yes please. ---
[jira] [Commented] (THRIFT-4447) Golang: Panic on p.c.Call when using deprecated initializers
[ https://issues.apache.org/jira/browse/THRIFT-4447?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16318830#comment-16318830 ] ASF GitHub Bot commented on THRIFT-4447: Github user johnboiles commented on the issue: https://github.com/apache/thrift/pull/1461 @dcelasun any thoughts on fixing the tests? Looks like `tutorial/go/src/tutorial/tutorial.go` imports its `BaseClient` from another package. Since `c` isn't public in that other package, my modified initializer fails. Probably the simplest fix would just be to add a public accessor for `c`, (e.g. `func (p *BaseClient) C() thrift.TClient`) and us that public access in my modified initializer methods. What do you think? > Golang: Panic on p.c.Call when using deprecated initializers > > > Key: THRIFT-4447 > URL: https://issues.apache.org/jira/browse/THRIFT-4447 > Project: Thrift > Issue Type: Bug > Components: Go - Compiler >Affects Versions: 0.11.0 >Reporter: John Boiles > > Latest thrift:master can cause panics when using deprecated > `New*ClientFactory` and `New*ClientProtocol` functions. This happens because > both the Client and the BaseClient have an instance of a {{thrift.TClient}}. > The deprecated methods initialize the BaseClient's TClient, but other methods > use the Client's {{TClient}}. > For example, current thrift master generates structs like this > {code} > type MyServiceClient struct { > c thrift.TClient > *MyServiceBaseClient > } > type MyServiceBaseClient struct { > c thrift.TClient > } > {code} > And also a method like this: > {code} > func NewMyServiceClientFactory(t thrift.TTransport, f > thrift.TProtocolFactory) *MyServiceClient { > return {MyServiceBaseClient: > NewMyServiceBaseClientFactory(t, f)} > } > {code} > If that method is used, later calls to service methods will panic, since > {{p.c}} is nil (the actual client was stored in {{p.BaseMyServiceClient.c}}). > {code} > func (p *MyServiceClient) DoStuff(ctx context.Context, request > *DoStuffRequest) (r *DoStuffResponse, err error) { > var _args139 DoStuffArgs > _args139.Request = request > var _result140 DoStuffResult > if err = p.c.Call(ctx, "do_stuff", &_args139, &_result140); err != nil > { // PANIC > ... > {code} > In progress fix here :https://github.com/apache/thrift/pull/1461. The fix in > this PR merely sets both instances of {{TClient}} (which is what happens in > the non-deprecated {{New*Client}} function). > This patch currently fails {{make -k check}} however, since > {{src/tutorial/tutorial.go}} tries to access a different package's version of > the BaseClient. > {code} > src/tutorial/tutorial.go:477:33: bc.c undefined (cannot refer to unexported > field or method c) > {code} > The fix for that test could possibly be to expose the BaseClient's instance > of {{c}} (by making it a capital and thus exported {{C}}), or adding an > accessor method {{C()}} or {{Client()}}. > Possibly a better fix would be to either remove these deprecated methods, or > figure out which {{TClient}} is the correct one to set. > I'm not sure which is preferred, so I'm hoping to get some feedback/input > here. -- This message was sent by Atlassian JIRA (v6.4.14#64029)
[GitHub] thrift issue #1461: THRIFT-4447: Golang: Fix panic on p.c.Call when using de...
Github user johnboiles commented on the issue: https://github.com/apache/thrift/pull/1461 @dcelasun any thoughts on fixing the tests? Looks like `tutorial/go/src/tutorial/tutorial.go` imports its `BaseClient` from another package. Since `c` isn't public in that other package, my modified initializer fails. Probably the simplest fix would just be to add a public accessor for `c`, (e.g. `func (p *BaseClient) C() thrift.TClient`) and us that public access in my modified initializer methods. What do you think? ---
[jira] [Commented] (THRIFT-4447) Golang: Panic on p.c.Call when using deprecated initializers
[ https://issues.apache.org/jira/browse/THRIFT-4447?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16318822#comment-16318822 ] ASF GitHub Bot commented on THRIFT-4447: Github user dcelasun commented on the issue: https://github.com/apache/thrift/pull/1461 > Why are they deprecated? I probably only overlook sth, so bear with me My apologies, it's an unfortunate result of the patch changing authors and things getting mixed up in the process. I should have removed the deprecated comments, especially since those constructors *are* compatible. The bugfix looks good. > Golang: Panic on p.c.Call when using deprecated initializers > > > Key: THRIFT-4447 > URL: https://issues.apache.org/jira/browse/THRIFT-4447 > Project: Thrift > Issue Type: Bug > Components: Go - Compiler >Affects Versions: 0.11.0 >Reporter: John Boiles > > Latest thrift:master can cause panics when using deprecated > `New*ClientFactory` and `New*ClientProtocol` functions. This happens because > both the Client and the BaseClient have an instance of a {{thrift.TClient}}. > The deprecated methods initialize the BaseClient's TClient, but other methods > use the Client's {{TClient}}. > For example, current thrift master generates structs like this > {code} > type MyServiceClient struct { > c thrift.TClient > *MyServiceBaseClient > } > type MyServiceBaseClient struct { > c thrift.TClient > } > {code} > And also a method like this: > {code} > func NewMyServiceClientFactory(t thrift.TTransport, f > thrift.TProtocolFactory) *MyServiceClient { > return {MyServiceBaseClient: > NewMyServiceBaseClientFactory(t, f)} > } > {code} > If that method is used, later calls to service methods will panic, since > {{p.c}} is nil (the actual client was stored in {{p.BaseMyServiceClient.c}}). > {code} > func (p *MyServiceClient) DoStuff(ctx context.Context, request > *DoStuffRequest) (r *DoStuffResponse, err error) { > var _args139 DoStuffArgs > _args139.Request = request > var _result140 DoStuffResult > if err = p.c.Call(ctx, "do_stuff", &_args139, &_result140); err != nil > { // PANIC > ... > {code} > In progress fix here :https://github.com/apache/thrift/pull/1461. The fix in > this PR merely sets both instances of {{TClient}} (which is what happens in > the non-deprecated {{New*Client}} function). > This patch currently fails {{make -k check}} however, since > {{src/tutorial/tutorial.go}} tries to access a different package's version of > the BaseClient. > {code} > src/tutorial/tutorial.go:477:33: bc.c undefined (cannot refer to unexported > field or method c) > {code} > The fix for that test could possibly be to expose the BaseClient's instance > of {{c}} (by making it a capital and thus exported {{C}}), or adding an > accessor method {{C()}} or {{Client()}}. > Possibly a better fix would be to either remove these deprecated methods, or > figure out which {{TClient}} is the correct one to set. > I'm not sure which is preferred, so I'm hoping to get some feedback/input > here. -- This message was sent by Atlassian JIRA (v6.4.14#64029)
[GitHub] thrift issue #1461: THRIFT-4447: Golang: fix for (deprecated) New*ClientFact...
Github user dcelasun commented on the issue: https://github.com/apache/thrift/pull/1461 > Why are they deprecated? I probably only overlook sth, so bear with me My apologies, it's an unfortunate result of the patch changing authors and things getting mixed up in the process. I should have removed the deprecated comments, especially since those constructors *are* compatible. The bugfix looks good. ---
[jira] [Created] (THRIFT-4448) Golang: do something with context.Context
John Boiles created THRIFT-4448: --- Summary: Golang: do something with context.Context Key: THRIFT-4448 URL: https://issues.apache.org/jira/browse/THRIFT-4448 Project: Thrift Issue Type: Task Components: Go - Library Affects Versions: 0.11.0 Reporter: John Boiles PR Here: https://github.com/apache/thrift/pull/1459 This patch wires through {{context.Context}} such that it can be used in in {{http.Request}}'s {{WithContext}} method. This allows Thrift HTTP requests to canceled or timed out via the context. This patch breaks support for go<1.7 so it's not ready to ship, but I'm hoping to get some direction on this. When does Thrift expect to drop support of go1.7? It looks like the current solution is to duplicate files that need to use {{golang.org/x/net/context}} and add a {{// +build !go1.7}} but duplication seems unsustainable as the {{context}} package is imported more places. Go 1.7 was released 15 August 2016. Given Golang has had significant performance improvements in most dot releases, I suspect most production services stay reasonably up to date. Here at Periscope/Twitter we're on go1.9.1, and we're a fairly large organization. -- This message was sent by Atlassian JIRA (v6.4.14#64029)
[GitHub] thrift issue #1461: Golang: fix for (deprecated) New*ClientFactory and New*C...
Github user johnboiles commented on the issue: https://github.com/apache/thrift/pull/1461 > Why are they deprecated? I probably only overlook sth, so bear with me That's a great question. Looks like they were marked deprecated by @dcelasun in #1382 as a part of THRIFT-4285. That discussion predates my involvement in Thrift. > Do we have a JIRA Ticket for this? If not, could you create one? Thanks for the nudge, I was just being lazy. Created as THRIFT-4447 ---
[jira] [Commented] (THRIFT-4447) Golang: Panic on p.c.Call when using deprecated initializers
[ https://issues.apache.org/jira/browse/THRIFT-4447?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16318817#comment-16318817 ] John Boiles commented on THRIFT-4447: - Looks like the regression came with https://github.com/apache/thrift/pull/1382 > Golang: Panic on p.c.Call when using deprecated initializers > > > Key: THRIFT-4447 > URL: https://issues.apache.org/jira/browse/THRIFT-4447 > Project: Thrift > Issue Type: Bug > Components: Go - Compiler >Affects Versions: 0.11.0 >Reporter: John Boiles > > Latest thrift:master can cause panics when using deprecated > `New*ClientFactory` and `New*ClientProtocol` functions. This happens because > both the Client and the BaseClient have an instance of a {{thrift.TClient}}. > The deprecated methods initialize the BaseClient's TClient, but other methods > use the Client's {{TClient}}. > For example, current thrift master generates structs like this > {code} > type MyServiceClient struct { > c thrift.TClient > *MyServiceBaseClient > } > type MyServiceBaseClient struct { > c thrift.TClient > } > {code} > And also a method like this: > {code} > func NewMyServiceClientFactory(t thrift.TTransport, f > thrift.TProtocolFactory) *MyServiceClient { > return {MyServiceBaseClient: > NewMyServiceBaseClientFactory(t, f)} > } > {code} > If that method is used, later calls to service methods will panic, since > {{p.c}} is nil (the actual client was stored in {{p.BaseMyServiceClient.c}}). > {code} > func (p *MyServiceClient) DoStuff(ctx context.Context, request > *DoStuffRequest) (r *DoStuffResponse, err error) { > var _args139 DoStuffArgs > _args139.Request = request > var _result140 DoStuffResult > if err = p.c.Call(ctx, "do_stuff", &_args139, &_result140); err != nil > { // PANIC > ... > {code} > In progress fix here :https://github.com/apache/thrift/pull/1461. The fix in > this PR merely sets both instances of {{TClient}} (which is what happens in > the non-deprecated {{New*Client}} function). > This patch currently fails {{make -k check}} however, since > {{src/tutorial/tutorial.go}} tries to access a different package's version of > the BaseClient. > {code} > src/tutorial/tutorial.go:477:33: bc.c undefined (cannot refer to unexported > field or method c) > {code} > The fix for that test could possibly be to expose the BaseClient's instance > of {{c}} (by making it a capital and thus exported {{C}}), or adding an > accessor method {{C()}} or {{Client()}}. > Possibly a better fix would be to either remove these deprecated methods, or > figure out which {{TClient}} is the correct one to set. > I'm not sure which is preferred, so I'm hoping to get some feedback/input > here. -- This message was sent by Atlassian JIRA (v6.4.14#64029)
[jira] [Commented] (THRIFT-4285) Pull generated send/recv into library to allow behaviour to be customised
[ https://issues.apache.org/jira/browse/THRIFT-4285?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16318818#comment-16318818 ] ASF GitHub Bot commented on THRIFT-4285: Github user johnboiles commented on the issue: https://github.com/apache/thrift/pull/1461 > Why are they deprecated? I probably only overlook sth, so bear with me That's a great question. Looks like they were marked deprecated by @dcelasun in #1382 as a part of THRIFT-4285. That discussion predates my involvement in Thrift. > Do we have a JIRA Ticket for this? If not, could you create one? Thanks for the nudge, I was just being lazy. Created as THRIFT-4447 > Pull generated send/recv into library to allow behaviour to be customised > - > > Key: THRIFT-4285 > URL: https://issues.apache.org/jira/browse/THRIFT-4285 > Project: Thrift > Issue Type: Improvement > Components: Go - Compiler, Go - Library >Reporter: Chris Bannister >Assignee: Can Celasun > Fix For: 0.11.0 > > Attachments: 0001-go-pull-generated-send-recv-into-lib-v6.patch, > 0001-go-pull-generated-send-recv-into-lib-v7.patch > > > Currently it is difficult to change how thrift writes messages onto the > transport because they are in the generated code. Instead the generated > send/recv methods should be in the library. This will greatly simplify the > client code and remove many duplicate methods whilst allowing users more > flexibility to implement connection pools and other features such as THeader. -- This message was sent by Atlassian JIRA (v6.4.14#64029)
[jira] [Created] (THRIFT-4447) Golang: Panic on p.c.Call when using deprecated initializers
John Boiles created THRIFT-4447: --- Summary: Golang: Panic on p.c.Call when using deprecated initializers Key: THRIFT-4447 URL: https://issues.apache.org/jira/browse/THRIFT-4447 Project: Thrift Issue Type: Bug Components: Go - Compiler Affects Versions: 0.11.0 Reporter: John Boiles Latest thrift:master can cause panics when using deprecated `New*ClientFactory` and `New*ClientProtocol` functions. This happens because both the Client and the BaseClient have an instance of a {{thrift.TClient}}. The deprecated methods initialize the BaseClient's TClient, but other methods use the Client's {{TClient}}. For example, current thrift master generates structs like this {code} type MyServiceClient struct { c thrift.TClient *MyServiceBaseClient } type MyServiceBaseClient struct { c thrift.TClient } {code} And also a method like this: {code} func NewMyServiceClientFactory(t thrift.TTransport, f thrift.TProtocolFactory) *MyServiceClient { return {MyServiceBaseClient: NewMyServiceBaseClientFactory(t, f)} } {code} If that method is used, later calls to service methods will panic, since {{p.c}} is nil (the actual client was stored in {{p.BaseMyServiceClient.c}}). {code} func (p *MyServiceClient) DoStuff(ctx context.Context, request *DoStuffRequest) (r *DoStuffResponse, err error) { var _args139 DoStuffArgs _args139.Request = request var _result140 DoStuffResult if err = p.c.Call(ctx, "do_stuff", &_args139, &_result140); err != nil { // PANIC ... {code} In progress fix here :https://github.com/apache/thrift/pull/1461. The fix in this PR merely sets both instances of {{TClient}} (which is what happens in the non-deprecated {{New*Client}} function). This patch currently fails {{make -k check}} however, since {{src/tutorial/tutorial.go}} tries to access a different package's version of the BaseClient. {code} src/tutorial/tutorial.go:477:33: bc.c undefined (cannot refer to unexported field or method c) {code} The fix for that test could possibly be to expose the BaseClient's instance of {{c}} (by making it a capital and thus exported {{C}}), or adding an accessor method {{C()}} or {{Client()}}. Possibly a better fix would be to either remove these deprecated methods, or figure out which {{TClient}} is the correct one to set. I'm not sure which is preferred, so I'm hoping to get some feedback/input here. -- This message was sent by Atlassian JIRA (v6.4.14#64029)
[jira] [Commented] (THRIFT-4285) Pull generated send/recv into library to allow behaviour to be customised
[ https://issues.apache.org/jira/browse/THRIFT-4285?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16318804#comment-16318804 ] ASF GitHub Bot commented on THRIFT-4285: Github user johnboiles commented on a diff in the pull request: https://github.com/apache/thrift/pull/1382#discussion_r160477076 --- Diff: compiler/cpp/src/thrift/generate/t_go_generator.cc --- @@ -1953,177 +1960,75 @@ void t_go_generator::generate_service_client(t_service* tservice) { f_types_ << indent() << "func (p *" << serviceName << "Client) " << function_signature_if(*f_iter, "", true) << " {" << endl; indent_up(); -/* -f_types_ << - indent() << "p.SeqId += 1" << endl; -if (!(*f_iter)->is_oneway()) { - f_types_ << -indent() << "d := defer.Deferred()" << endl << -indent() << "p.Reqs[p.SeqId] = d" << endl; -} -*/ -f_types_ << indent() << "if err = p.send" << funname << "("; -bool first = true; - -for (fld_iter = fields.begin(); fld_iter != fields.end(); ++fld_iter) { - if (first) { -first = false; - } else { -f_types_ << ", "; - } - f_types_ << variable_name_to_go_name((*fld_iter)->get_name()); -} - -f_types_ << "); err != nil { return }" << endl; - -if (!(*f_iter)->is_oneway()) { - f_types_ << indent() << "return p.recv" << funname << "()" << endl; -} else { - f_types_ << indent() << "return" << endl; -} - -indent_down(); -f_types_ << indent() << "}" << endl << endl; -f_types_ << indent() << "func (p *" << serviceName << "Client) send" - << function_signature(*f_iter) << "(err error) {" << endl; -indent_up(); -std::string argsname = publicize((*f_iter)->get_name() + "_args", true); -// Serialize the request header -f_types_ << indent() << "oprot := p.OutputProtocol" << endl; -f_types_ << indent() << "if oprot == nil {" << endl; -f_types_ << indent() << " oprot = p.ProtocolFactory.GetProtocol(p.Transport)" << endl; -f_types_ << indent() << " p.OutputProtocol = oprot" << endl; -f_types_ << indent() << "}" << endl; -f_types_ << indent() << "p.SeqId++" << endl; -f_types_ << indent() << "if err = oprot.WriteMessageBegin(\"" << (*f_iter)->get_name() - << "\", " << ((*f_iter)->is_oneway() ? "thrift.ONEWAY" : "thrift.CALL") - << ", p.SeqId); err != nil {" << endl; -indent_up(); -f_types_ << indent() << " return" << endl; -indent_down(); -f_types_ << indent() << "}" << endl; -f_types_ << indent() << "args := " << argsname << "{" << endl; +std::string method = (*f_iter)->get_name(); +std::string argsType = publicize(method + "_args", true); +std::string argsName = tmp("_args"); +f_types_ << indent() << "var " << argsName << " " << argsType << endl; for (fld_iter = fields.begin(); fld_iter != fields.end(); ++fld_iter) { - f_types_ << indent() << publicize((*fld_iter)->get_name()) << " : " - << variable_name_to_go_name((*fld_iter)->get_name()) << "," << endl; + f_types_ << indent() << argsName << "." << publicize((*fld_iter)->get_name()) + << " = " << variable_name_to_go_name((*fld_iter)->get_name()) << endl; } -f_types_ << indent() << "}" << endl; - -// Write to the stream -f_types_ << indent() << "if err = args." << write_method_name_ << "(oprot); err != nil {" << endl; -indent_up(); -f_types_ << indent() << " return" << endl; -indent_down(); -f_types_ << indent() << "}" << endl; -f_types_ << indent() << "if err = oprot.WriteMessageEnd(); err != nil {" << endl; -indent_up(); -f_types_ << indent() << " return" << endl; -indent_down(); -f_types_ << indent() << "}" << endl; -f_types_ << indent() << "return oprot.Flush()" << endl; -indent_down(); -f_types_ << indent() << "}" << endl << endl; if (!(*f_iter)->is_oneway()) { - std::string resultname = publicize((*f_iter)->get_name() + "_result", true); - // Open function - f_types_ << endl << indent() << "func (p *" << serviceName << "Client) recv" - << publicize((*f_iter)->get_name()) << "() ("; + 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.c.Call(ctx, \"" --- End diff -- When using one of the now-deprecated
[GitHub] thrift pull request #1382: THRIFT-4285 Move TX/RX methods from gen. code to ...
Github user johnboiles commented on a diff in the pull request: https://github.com/apache/thrift/pull/1382#discussion_r160477076 --- Diff: compiler/cpp/src/thrift/generate/t_go_generator.cc --- @@ -1953,177 +1960,75 @@ void t_go_generator::generate_service_client(t_service* tservice) { f_types_ << indent() << "func (p *" << serviceName << "Client) " << function_signature_if(*f_iter, "", true) << " {" << endl; indent_up(); -/* -f_types_ << - indent() << "p.SeqId += 1" << endl; -if (!(*f_iter)->is_oneway()) { - f_types_ << -indent() << "d := defer.Deferred()" << endl << -indent() << "p.Reqs[p.SeqId] = d" << endl; -} -*/ -f_types_ << indent() << "if err = p.send" << funname << "("; -bool first = true; - -for (fld_iter = fields.begin(); fld_iter != fields.end(); ++fld_iter) { - if (first) { -first = false; - } else { -f_types_ << ", "; - } - f_types_ << variable_name_to_go_name((*fld_iter)->get_name()); -} - -f_types_ << "); err != nil { return }" << endl; - -if (!(*f_iter)->is_oneway()) { - f_types_ << indent() << "return p.recv" << funname << "()" << endl; -} else { - f_types_ << indent() << "return" << endl; -} - -indent_down(); -f_types_ << indent() << "}" << endl << endl; -f_types_ << indent() << "func (p *" << serviceName << "Client) send" - << function_signature(*f_iter) << "(err error) {" << endl; -indent_up(); -std::string argsname = publicize((*f_iter)->get_name() + "_args", true); -// Serialize the request header -f_types_ << indent() << "oprot := p.OutputProtocol" << endl; -f_types_ << indent() << "if oprot == nil {" << endl; -f_types_ << indent() << " oprot = p.ProtocolFactory.GetProtocol(p.Transport)" << endl; -f_types_ << indent() << " p.OutputProtocol = oprot" << endl; -f_types_ << indent() << "}" << endl; -f_types_ << indent() << "p.SeqId++" << endl; -f_types_ << indent() << "if err = oprot.WriteMessageBegin(\"" << (*f_iter)->get_name() - << "\", " << ((*f_iter)->is_oneway() ? "thrift.ONEWAY" : "thrift.CALL") - << ", p.SeqId); err != nil {" << endl; -indent_up(); -f_types_ << indent() << " return" << endl; -indent_down(); -f_types_ << indent() << "}" << endl; -f_types_ << indent() << "args := " << argsname << "{" << endl; +std::string method = (*f_iter)->get_name(); +std::string argsType = publicize(method + "_args", true); +std::string argsName = tmp("_args"); +f_types_ << indent() << "var " << argsName << " " << argsType << endl; for (fld_iter = fields.begin(); fld_iter != fields.end(); ++fld_iter) { - f_types_ << indent() << publicize((*fld_iter)->get_name()) << " : " - << variable_name_to_go_name((*fld_iter)->get_name()) << "," << endl; + f_types_ << indent() << argsName << "." << publicize((*fld_iter)->get_name()) + << " = " << variable_name_to_go_name((*fld_iter)->get_name()) << endl; } -f_types_ << indent() << "}" << endl; - -// Write to the stream -f_types_ << indent() << "if err = args." << write_method_name_ << "(oprot); err != nil {" << endl; -indent_up(); -f_types_ << indent() << " return" << endl; -indent_down(); -f_types_ << indent() << "}" << endl; -f_types_ << indent() << "if err = oprot.WriteMessageEnd(); err != nil {" << endl; -indent_up(); -f_types_ << indent() << " return" << endl; -indent_down(); -f_types_ << indent() << "}" << endl; -f_types_ << indent() << "return oprot.Flush()" << endl; -indent_down(); -f_types_ << indent() << "}" << endl << endl; if (!(*f_iter)->is_oneway()) { - std::string resultname = publicize((*f_iter)->get_name() + "_result", true); - // Open function - f_types_ << endl << indent() << "func (p *" << serviceName << "Client) recv" - << publicize((*f_iter)->get_name()) << "() ("; + 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.c.Call(ctx, \"" --- End diff -- When using one of the now-deprecated initializers, this line will cause a panic, since `p.c` is nil. The deprecated initializers create the `TClient` at `p.BaseMyServiceClient.c` ---
[jira] [Created] (THRIFT-4446) JSONProtocol Base64 Encoding Trims Padding
Allen created THRIFT-4446: - Summary: JSONProtocol Base64 Encoding Trims Padding Key: THRIFT-4446 URL: https://issues.apache.org/jira/browse/THRIFT-4446 Project: Thrift Issue Type: Bug Components: .NETCore - Library, C# - Library Affects Versions: 0.11.0 Reporter: Allen In the C# and .NET Core libraries, the JSONProtocol's Binary Encoding to Base64 trims padding from the user provided byte arrays before encoding into Base64. This behavior is incorrect, as the user provided data should be encoded exactly as provided. Otherwise, data may be lost. Fixed by no longer trimming padding on encode. Padding must still be trimmed on decode, in accordance with the Base64 specification. For example: * Before this patch, encoding the byte array [0x01, 0x3d, 0x3d] yields [0x01] upon decode. This is incorrect, as I should decode the exact data that I encoded. * After this patch, it yields [0x01, 0x3d, 0x3d], as expected. I have submitted a pull request [here|https://github.com/apache/thrift/pull/1463] -- This message was sent by Atlassian JIRA (v6.4.14#64029)
[GitHub] thrift pull request #1463: JSONProtocol Base64 Encoding: Do not trim padding...
GitHub user sithas opened a pull request: https://github.com/apache/thrift/pull/1463 JSONProtocol Base64 Encoding: Do not trim padding on encode. * In the C# and .NET Core libraries, the JSONProtocol's Binary Encoding to Base64 trims padding from the user provided byte arrays before encoding into Base64. This behavior is incorrect, as the user provided data should be encoded exactly as provided. Otherwise, data may be lost. * Fixed by no longer trimming padding on encode. Padding must still be trimmed on decode, in accordance with the Base64 specification. * For example: * Before this patch, encoding the byte array `[0x01, 0x3d, 0x3d]` yields `[0x01]` upon decode. This is incorrect, as I should decode the exact data that I encoded. * After this patch, it yields `[0x01, 0x3d, 0x3d]`, as expected. You can merge this pull request into a Git repository by running: $ git pull https://github.com/sithas/thrift master Alternatively you can review and apply these changes as the patch at: https://github.com/apache/thrift/pull/1463.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #1463 commit 4cdf1f4a332d5f14923fdc2929321664de1d3c8d Author: Allen WarthenDate: 2018-01-09T17:04:14Z JSONProtocol Base64 Encoding: Do not trim padding on encode. - In the C# and .NET Core libraries, the JSONProtocol's Binary Encoding to Base64 trims padding from the user provided byte arrays before encoding into base64. This behaviour is incorrect, as the user provided bytes should be encoded exactly as they are provided. Otherwise, data may be lost. - Fixed by no longer trimming padding on encode. ---
[GitHub] thrift issue #1461: Golang: fix for (deprecated) New*ClientFactory and New*C...
Github user Jens-G commented on the issue: https://github.com/apache/thrift/pull/1461 Hi John, Questions: * Why are they deprecated? I probably only overlook sth, so bear with me * Do we have a JIRA Ticket for this? If not, could you create one? ---