[GitHub] thrift issue #1382: THRIFT-4285 Move TX/RX methods from gen. code to library
Github user jeking3 commented on the issue: https://github.com/apache/thrift/pull/1382 Since it's just documentation if you want to add it as a patch to a ticket instead of putting it through CI, someone can merge it from there. ---
[GitHub] thrift issue #1382: THRIFT-4285 Move TX/RX methods from gen. code to library
Github user dcelasun commented on the issue: https://github.com/apache/thrift/pull/1382 > That needs to be documented in the dlang readme - would you be able to document that in a separate PR? Sorry for the delay. I'll open a ticket and send a patch over the weekend. ---
[GitHub] thrift issue #1382: THRIFT-4285 Move TX/RX methods from gen. code to library
Github user jeking3 commented on the issue: https://github.com/apache/thrift/pull/1382 Excellent, I will merge this, and we'll get it into 0.11.0 - so yay! We can consider the backwards-compatible construction adapter to be deprecated so that it can be removed in the follow-on release. That needs to be documented in the dlang readme - would you be able to document that in a separate PR? Since it's just a readme if you want to make it a patch on a thrift jira ticket that would be enough to merge in the readme change. ---
[GitHub] thrift issue #1382: THRIFT-4285 Move TX/RX methods from gen. code to library
Github user dcelasun commented on the issue: https://github.com/apache/thrift/pull/1382 @jeking3 All done! I think we are good to merge, what do you think? ---
[GitHub] thrift issue #1382: THRIFT-4285 Move TX/RX methods from gen. code to library
Github user jeking3 commented on the issue: https://github.com/apache/thrift/pull/1382 0.11.0 cycle hasn't started yet - you still have some time (I don't know how much). ---
[GitHub] thrift issue #1382: THRIFT-4285 Move TX/RX methods from gen. code to library
Github user jeking3 commented on the issue: https://github.com/apache/thrift/pull/1382 Nothing I know of. @jfarrell manages release schedules. ---
[GitHub] thrift issue #1382: THRIFT-4285 Move TX/RX methods from gen. code to library
Github user dcelasun commented on the issue: https://github.com/apache/thrift/pull/1382 @jeking3 Is there a deadline for 0.11? I really want to get this into the next release, but likely won't have enough time to finish it up in the next week or so. ---
[GitHub] thrift issue #1382: THRIFT-4285 Move TX/RX methods from gen. code to library
Github user jeking3 commented on the issue: https://github.com/apache/thrift/pull/1382 Given the backlog on the project, I don't blame you! :) ---
[GitHub] thrift issue #1382: THRIFT-4285 Move TX/RX methods from gen. code to library
Github user dcelasun commented on the issue: https://github.com/apache/thrift/pull/1382 > Is there a way you can provide a generated NewFooClientFactory as an adapter to run the new code? It would require putting back some of the generated code, but I'll try to find a way. I agree avoiding the BC break is worth it if possible. > I was away for a couple days on college tours with my son, sorry for the delay. No worries, I just wanted to ping you in case this slipped through the cracks. ---
[GitHub] thrift issue #1382: THRIFT-4285 Move TX/RX methods from gen. code to library
Github user jeking3 commented on the issue:
https://github.com/apache/thrift/pull/1382
Is there a way you can provide a generated NewFooClientFactory as an
adapter to run the new code? i.e. (sorry this isn't go, but you get the idea):
mypkg.NewFooClientFactory(transport, protocolFactory) { return
mypkg.NewFooClient(thrift.NewTStandardClient(protocolFactory.GetProtocol(transport)));
}
This would make it backwards compatible for everyone and you wouldn't need
to change any of the test files, I believe? This would be better overall for
the project to maintain backwards compatibility. Let me know what you think.
I was away for a couple days on college tours with my son, sorry for the
delay.
---
[GitHub] thrift issue #1382: THRIFT-4285 Move TX/RX methods from gen. code to library
Github user dcelasun commented on the issue: https://github.com/apache/thrift/pull/1382 ping @jeking3 ---
[GitHub] thrift issue #1382: THRIFT-4285 Move TX/RX methods from gen. code to library
Github user dcelasun commented on the issue:
https://github.com/apache/thrift/pull/1382
@jeking3 All tests green. I've reverted all the Go version changes and made
the tests compatible with Go 1.4.
I've also tested Thrift from this branch (both compiler and lib) with a
large, real project and everything works fine. Here's what I had to change:
IDL:
```thrift
package mypkg
service Foo {}
```
Old code:
```go
client := mypkg.NewFooClientFactory(transport, protocolFactory)
```
New code:
```go
protocol := protocolFactory.GetProtocol(transport)
client := mypkg.NewFooClient(thrift.NewTStandardClient(p, p))
```
That's it. There are other BC breaks on `master`, but this is the only
change needed for this patch.
If you are happy with it as well, I can squash and rebase from master.
---
[GitHub] thrift issue #1382: THRIFT-4285 Move TX/RX methods from gen. code to library
Github user dcelasun commented on the issue: https://github.com/apache/thrift/pull/1382 > I don't think we can unilaterally require go 1.9 at this point without causing some pain, but I'm not sure. This change doesn't effect the user, it's only needed for the tests. Though if you still think we shouldn't do it, I can change the tests to use older APIs. I didn't want to touch it since I didn't write the original patch. > It looks like this may not be backwards compatible with existing code - is there any way to put in an adapter that would allow existing code to continue working? I don't think so. Several interfaces had to change since what used to be returned from generated methods is now returned from the library. Good news is the changes are limited to client, server and protocol interfaces (they don't affect the signature for RPCs) so updating to 0.11 should be a few lines of change (initializing the server/client) for most people. Just to make sure, I'll update a real application to this patch and post my experiences here. ---
[GitHub] thrift issue #1382: THRIFT-4285 Move TX/RX methods from gen. code to library
Github user jeking3 commented on the issue: https://github.com/apache/thrift/pull/1382 We support go back to version 1.2.1 or 1.4.3 (I can't remember which, but I think 1.2.1 is on trusty). That said, I'm in the middle of reworking the docker images to be as stock as possible, and adding an ubuntu-artful one which has go 1.8.3 on it. I don't think we can unilaterally require go 1.9 at this point without causing some pain, but I'm not sure. I haven't been that involved in the go ecosystem. ---
[GitHub] thrift issue #1382: THRIFT-4285 Move TX/RX methods from gen. code to library
Github user dcelasun commented on the issue: https://github.com/apache/thrift/pull/1382 Travis failure is unrelated, all Go tests are green. cc @jeking3 thoughts? ---
