You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@thrift.apache.org by szenker <gi...@git.apache.org> on 2016/01/18 09:10:28 UTC
[GitHub] thrift pull request: C++: declared TTransport::isOpen() and TTrans...
GitHub user szenker opened a pull request:
https://github.com/apache/thrift/pull/798
C++: declared TTransport::isOpen() and TTransport::getOrigin() as const member functions
Also TTransport::peek() should be const in my opinion, but this requires some refactoring.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/szenker/thrift THRIFT-3560
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/thrift/pull/798.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 #798
----
commit 19ac70fc5e10ce7ee60877cc273972fccb41388f
Author: Sebastian Zenker <se...@gmx.de>
Date: 2016-01-18T07:48:20Z
C++: declared TTransport::isOpen() and TTransport::getOrigin() as const member functions. Also TTransport::peek() should be const in my opinion, but this requires some refactoring.
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request: THRIFT-3560: C++: declared TTransport::isOpen...
Posted by hcorg <gi...@git.apache.org>.
Github user hcorg commented on the pull request:
https://github.com/apache/thrift/pull/798#issuecomment-173236675
if it's a breaking change maybe we should put is as part of cpp_v2 developement? We could use override and other C++11 features to help users migrate.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request: THRIFT-3560: C++: declared TTransport::isOpen...
Posted by ben-craig <gi...@git.apache.org>.
Github user ben-craig commented on the pull request:
https://github.com/apache/thrift/pull/798#issuecomment-173401300
@szenker It's probably not what you are looking for, but the concurrent client in C++ can allow multiple threads to have outstanding requests at once (though only one request per thread). There isn't a concurrent server right now that does out-of-order replies, but the client side can handle it.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request: THRIFT-3560: C++: declared TTransport::isOpen...
Posted by szenker <gi...@git.apache.org>.
Github user szenker commented on the pull request:
https://github.com/apache/thrift/pull/798#issuecomment-173338811
With the term "pipelining" I mean the following in this context:
1.) A client is allowed to send out a consecutive request even before it has received the response of the preceding request.
2.) When a server receives several requests from a particular client, it may sends back the responses in a different order than it received the requests.
This doesn't work with the current cob style implementation, as the seqid is set to 0 in all requests/reponses.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request: THRIFT-3560: C++: declared TTransport::isOpen...
Posted by jeking3 <gi...@git.apache.org>.
Github user jeking3 commented on the pull request:
https://github.com/apache/thrift/pull/798#issuecomment-173236071
I'm a fan of const correctness. We shouldn't penalize const correctness due to an inherent weakness in the language (that was resolved in C++11, i.e. the [override operator](http://en.cppreference.com/w/cpp/language/override)). I agree this would be something to list as part of a release's "breaking changes" which would affect customer created subclasses.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift issue #798: THRIFT-3560: C++: declared TTransport::isOpen() and TTran...
Posted by jeking3 <gi...@git.apache.org>.
Github user jeking3 commented on the issue:
https://github.com/apache/thrift/pull/798
I am okay making this change for 0.12.0; if you can rebase, squash, and fix up the issues and force push, and if there are any other methods that need the same const correctness, let's do those too.
---
[GitHub] thrift pull request: THRIFT-3560: C++: declared TTransport::isOpen...
Posted by nsuke <gi...@git.apache.org>.
Github user nsuke commented on the pull request:
https://github.com/apache/thrift/pull/798#issuecomment-173229082
Implementation aside, this change breaks users' existing custom transports, although mostly in trivial ways.
What is your (hypothetical) use case(s) with this or peek const change ?
Would this make the API more usable ?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request: THRIFT-3560: C++: declared TTransport::isOpen...
Posted by szenker <gi...@git.apache.org>.
Github user szenker commented on the pull request:
https://github.com/apache/thrift/pull/798#issuecomment-173337036
Adding the const keyword is not just syntactical sugar. It requires users which derive from TTransport to be more strict when they implement their own version of isOpen() and getOrigin() as it enforces them to not alter the internal state of their custom TTransport implementation when we are calling one of those methods from the Thrift C++ library. So from the Thrift library point of view, we can refactor the C++ library and call isOpen() more or less often without introducing side effects in the users code. peek() should be also const in my opinion, as it should return true in case data is available for reading.
@hcorg: I took a quick look into your fork on github (cpp_v2). I really like the way how you refactored the c++ code generator. How can I get involved in the refactoring of the cpp_v2 library / compiler? Especially I'm interested in supporting to refactor the asynchronous client and server. E.g. to add exception support and the ability to do pipelining.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request: THRIFT-3560: C++: declared TTransport::isOpen...
Posted by hcorg <gi...@git.apache.org>.
Github user hcorg commented on the pull request:
https://github.com/apache/thrift/pull/798#issuecomment-173373046
@szenker pull requests are welcome :) right now I'm trying to revive my old branch (I'm porting some changes done to cpp lib into cpp_v2), currently it might not even compile :/ but c++11/14 is something I really want to enable for thrift. std::future for asynchronous tasks.. std::optional for optionals (c++17, but still :) ). I'll try to set up some discussion on dev@ mailing list later this week/weekend so we might split some tasks etc.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request: THRIFT-3560: C++: declared TTransport::isOpen...
Posted by nsuke <gi...@git.apache.org>.
Github user nsuke commented on the pull request:
https://github.com/apache/thrift/pull/798#issuecomment-174161478
@szenker thanks for elaborating.
I believe a minor ease of refactoring is not enough for silently ignored (non-)overrides.
But if your plan of `peek` solves an existing nasty pitfall or something, please do share your thought.
Also I agree that it's not much of an issue for _v2.
A minor nit: if we're changing `getOrigin` signature at all, I think the return type should be changed to non-const (or reference).
And another thing to consider for const under C++11: it now arguably signifies non data race guarantee.
http://herbsutter.com/2013/01/01/video-you-dont-know-const-and-mutable/
For example `TSocket::getOrigin` involves some system calls and internal caching that we may need synchronization.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift issue #798: THRIFT-3560: C++: declared TTransport::isOpen() and TTran...
Posted by jeking3 <gi...@git.apache.org>.
Github user jeking3 commented on the issue:
https://github.com/apache/thrift/pull/798
We need to decide if we move forward with this or not. I'm in favor of const correctness here however it is one of those things you need to do from the beginning because it changes the ABI. We have a prettu good test suite for this project and I think the only possible issue is that user's subclasses of these bases might no longer override what they expect to override. As with all breaking changes however it would be documented in the readme for lib/cpp.
---
[GitHub] thrift pull request: THRIFT-3560: C++: declared TTransport::isOpen...
Posted by jeking3 <gi...@git.apache.org>.
Github user jeking3 commented on the pull request:
https://github.com/apache/thrift/pull/798#issuecomment-173449026
I posted a C# runtime rewrite about 5 years ago that allowed for two way communication and multiple outstanding requests with out-of-order replies using the sequence number in the binary V1 header... it also had multiple channel support similar to the multiplexing protocol except I built it into the binary protocol. It is in Apache Jira under THRIFT-66. Perhaps you can leverage some concepts from that work.
As for the const correctness issue, I'm okay with the proposed changes going into the current implementation.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---