You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@thrift.apache.org by dcelasun <gi...@git.apache.org> on 2017/10/02 12:28:13 UTC
[GitHub] thrift pull request #1382: THRIFT-4285 Move TX/RX methods from gen. code to ...
GitHub user dcelasun opened a pull request:
https://github.com/apache/thrift/pull/1382
THRIFT-4285 Move TX/RX methods from gen. code to library
This change removes a lot of duplication from generated code and allows the caller to customize how they can read from / write to the transport.
This patch was originally written by [Chris Bannister](https://issues.apache.org/jira/browse/THRIFT-4285) but it seemed abandoned and no longer applied cleanly to master. I fixed it in order to get things moving again.
I've also bumped `Dockerfile`s to Go 1.9 since `t.Run` in `testing/T` doesn't exist before that and we were already using 1.9 for the CentOS container.
It would be great if this can be merged before 0.11 is tagged.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/dcelasun/thrift THRIFT-4285
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/thrift/pull/1382.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 #1382
----
----
---
[GitHub] thrift issue #1382: THRIFT-4285 Move TX/RX methods from gen. code to library
Posted by jeking3 <gi...@git.apache.org>.
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
Posted by dcelasun <gi...@git.apache.org>.
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?
---
[GitHub] thrift pull request #1382: THRIFT-4285 Move TX/RX methods from gen. code to ...
Posted by dcelasun <gi...@git.apache.org>.
Github user dcelasun commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1382#discussion_r148798731
--- Diff: build/docker/ubuntu-trusty/Dockerfile ---
@@ -217,6 +217,62 @@ RUN rm -rf /var/cache/apt/* && \
rm -rf /tmp/* && \
rm -rf /var/tmp/*
+# Ruby
--- End diff --
Fixed.
---
[GitHub] thrift pull request #1382: THRIFT-4285 Move TX/RX methods from gen. code to ...
Posted by jeking3 <gi...@git.apache.org>.
Github user jeking3 commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1382#discussion_r148793005
--- Diff: build/docker/ubuntu-trusty/Dockerfile ---
@@ -217,6 +217,62 @@ RUN rm -rf /var/cache/apt/* && \
rm -rf /tmp/* && \
rm -rf /var/tmp/*
+# Ruby
--- End diff --
This change looks incorrect. It duplicates other parts of the file.
---
[GitHub] thrift issue #1382: THRIFT-4285 Move TX/RX methods from gen. code to library
Posted by dcelasun <gi...@git.apache.org>.
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
Posted by jeking3 <gi...@git.apache.org>.
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
Posted by dcelasun <gi...@git.apache.org>.
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 pull request #1382: THRIFT-4285 Move TX/RX methods from gen. code to ...
Posted by dcelasun <gi...@git.apache.org>.
Github user dcelasun commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1382#discussion_r148797018
--- Diff: lib/go/test/tests/client_error_test.go ---
@@ -411,7 +411,7 @@ func TestClientReportTTransportErrors(t *testing.T) {
if !prepareClientCallReply(protocol, i, err) {
return
}
- client := errortest.NewErrorTestClientProtocol(transport, protocol, protocol)
+ client := errortest.NewErrorTestClient(thrift.NewTStandardClient(protocol, protocol))
--- End diff --
No, it's backwards compatible now (see [here](https://github.com/dcelasun/thrift/blob/555efe5aefe9619a900471e56e86906d40bc96b9/compiler/cpp/src/thrift/generate/t_go_generator.cc#L1889-L1930)), the test simply uses the new method.
I've also tested it with the integration of tests of a real, production app. It works as expected.
---
[GitHub] thrift pull request #1382: THRIFT-4285 Move TX/RX methods from gen. code to ...
Posted by jeking3 <gi...@git.apache.org>.
Github user jeking3 commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1382#discussion_r148808038
--- Diff: lib/go/test/tests/client_error_test.go ---
@@ -411,7 +411,7 @@ func TestClientReportTTransportErrors(t *testing.T) {
if !prepareClientCallReply(protocol, i, err) {
return
}
- client := errortest.NewErrorTestClientProtocol(transport, protocol, protocol)
+ client := errortest.NewErrorTestClient(thrift.NewTStandardClient(protocol, protocol))
--- End diff --
Not having to change the test is a better proof that the original method works :)
---
[GitHub] thrift issue #1382: THRIFT-4285 Move TX/RX methods from gen. code to library
Posted by dcelasun <gi...@git.apache.org>.
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 pull request #1382: THRIFT-4285 Move TX/RX methods from gen. code to ...
Posted by johnboiles <gi...@git.apache.org>.
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`
---
[GitHub] thrift pull request #1382: THRIFT-4285 Move TX/RX methods from gen. code to ...
Posted by dcelasun <gi...@git.apache.org>.
Github user dcelasun commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1382#discussion_r148808675
--- Diff: lib/go/test/tests/client_error_test.go ---
@@ -411,7 +411,7 @@ func TestClientReportTTransportErrors(t *testing.T) {
if !prepareClientCallReply(protocol, i, err) {
return
}
- client := errortest.NewErrorTestClientProtocol(transport, protocol, protocol)
+ client := errortest.NewErrorTestClient(thrift.NewTStandardClient(protocol, protocol))
--- End diff --
Good point :) I'll add both, but it'll have to wait till tomorrow as I've already left the office.
---
[GitHub] thrift issue #1382: THRIFT-4285 Move TX/RX methods from gen. code to library
Posted by jeking3 <gi...@git.apache.org>.
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
Posted by jeking3 <gi...@git.apache.org>.
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
Posted by jeking3 <gi...@git.apache.org>.
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 pull request #1382: THRIFT-4285 Move TX/RX methods from gen. code to ...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/thrift/pull/1382
---
[GitHub] thrift issue #1382: THRIFT-4285 Move TX/RX methods from gen. code to library
Posted by dcelasun <gi...@git.apache.org>.
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
Posted by jeking3 <gi...@git.apache.org>.
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
Posted by jeking3 <gi...@git.apache.org>.
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 pull request #1382: THRIFT-4285 Move TX/RX methods from gen. code to ...
Posted by jeking3 <gi...@git.apache.org>.
Github user jeking3 commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1382#discussion_r148808549
--- Diff: build/docker/ubuntu-trusty/Dockerfile ---
@@ -221,3 +221,4 @@ ENV THRIFT_ROOT /thrift
RUN mkdir -p $THRIFT_ROOT/src
COPY Dockerfile $THRIFT_ROOT/
WORKDIR $THRIFT_ROOT/src
+
--- End diff --
This extra line will force an image rebuild.
---
[GitHub] thrift pull request #1382: THRIFT-4285 Move TX/RX methods from gen. code to ...
Posted by jeking3 <gi...@git.apache.org>.
Github user jeking3 commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1382#discussion_r148793389
--- Diff: lib/go/test/tests/client_error_test.go ---
@@ -411,7 +411,7 @@ func TestClientReportTTransportErrors(t *testing.T) {
if !prepareClientCallReply(protocol, i, err) {
return
}
- client := errortest.NewErrorTestClientProtocol(transport, protocol, protocol)
+ client := errortest.NewErrorTestClient(thrift.NewTStandardClient(protocol, protocol))
--- End diff --
Does this means the change is still not backwards compatible with previously written customer code?
---
[GitHub] thrift issue #1382: THRIFT-4285 Move TX/RX methods from gen. code to library
Posted by dcelasun <gi...@git.apache.org>.
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
Posted by dcelasun <gi...@git.apache.org>.
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 pull request #1382: THRIFT-4285 Move TX/RX methods from gen. code to ...
Posted by dcelasun <gi...@git.apache.org>.
Github user dcelasun commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1382#discussion_r148796351
--- Diff: build/docker/ubuntu-trusty/Dockerfile ---
@@ -217,6 +217,62 @@ RUN rm -rf /var/cache/apt/* && \
rm -rf /tmp/* && \
rm -rf /var/tmp/*
+# Ruby
--- End diff --
It's because I messed up the rebase. Will fix.
---
[GitHub] thrift issue #1382: THRIFT-4285 Move TX/RX methods from gen. code to library
Posted by dcelasun <gi...@git.apache.org>.
Github user dcelasun commented on the issue:
https://github.com/apache/thrift/pull/1382
ping @jeking3
---
[GitHub] thrift pull request #1382: THRIFT-4285 Move TX/RX methods from gen. code to ...
Posted by dcelasun <gi...@git.apache.org>.
Github user dcelasun commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1382#discussion_r148808724
--- Diff: build/docker/ubuntu-trusty/Dockerfile ---
@@ -221,3 +221,4 @@ ENV THRIFT_ROOT /thrift
RUN mkdir -p $THRIFT_ROOT/src
COPY Dockerfile $THRIFT_ROOT/
WORKDIR $THRIFT_ROOT/src
+
--- End diff --
My bad, will revert this as well.
---