You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@thrift.apache.org by littletiny <gi...@git.apache.org> on 2014/12/11 07:27:30 UTC
[GitHub] thrift pull request: fix c++ version zeromq transport, the old ver...
GitHub user littletiny opened a pull request:
https://github.com/apache/thrift/pull/315
fix c++ version zeromq transport, the old version cannot work
fix c++ version zeromq transport, the old version cannot work
we need to override
```
TTransport::write_virt TTransport::read_virt
```
instead of
```
TTransport::write TTransport::read
```
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/littletiny/thrift master
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/thrift/pull/315.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 #315
----
commit bf792bb89cab8e42bbde6e9965bef5c8f2deacd0
Author: tiny <lo...@gmail.com>
Date: 2014-12-11T06:22:29Z
fix c++ version zeromq transport, the old version cannot work
----
---
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: fix c++ version zeromq transport, the old ver...
Posted by Jens-G <gi...@git.apache.org>.
Github user Jens-G commented on the pull request:
https://github.com/apache/thrift/pull/315#issuecomment-66708667
Hi,
Firstly, thanks for the patch.
Could you please ...
- [File a JIRA ticket for it](http://thrift.apache.org/docs/HowToContribute)
- Explain the problem in more detail? Especially, why could it not work? I'm quite sure the code has been tested before it was committed ... [see here for details](https://issues.apache.org/jira/browse/THRIFT-812)
Have fun,
JensG
---
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: fix c++ version zeromq transport, the old ver...
Posted by Jens-G <gi...@git.apache.org>.
Github user Jens-G commented on the pull request:
https://github.com/apache/thrift/pull/315#issuecomment-67892304
Thanks for catching this.
PS: I created THRIFT-2911 for you. Please try to follow [our few guidelines](http://thrift.apache.org/docs/HowToContribute) when submitting the next PR. Thank you
---
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: fix c++ version zeromq transport, the old ver...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/thrift/pull/315
---
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: fix c++ version zeromq transport, the old ver...
Posted by nsuke <gi...@git.apache.org>.
Github user nsuke commented on the pull request:
https://github.com/apache/thrift/pull/315#issuecomment-67766086
TTransport inheritance made by TZmqClient is indeed incorrect, it doesn't override anything, only hiding virtual methods.
It should still work if used directly like TBinaryProtocolT\<TZmqClient\>, but does nothing when TTransport virtual methods are called.
Latter is what happens when "normal" TBinaryProtocol is used.
---
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: fix c++ version zeromq transport, the old ver...
Posted by littletiny <gi...@git.apache.org>.
Github user littletiny commented on the pull request:
https://github.com/apache/thrift/pull/315#issuecomment-66949477
the c++ version in directory contrib/zeromq override wrong interface
in TZmqClient.h
TZmqClient inherit from TTransport
TTransport provide these method:
// these two methods is not virtual !
read
write
// these tow methods is virtual
read_virt
write_virt
But in TZmqClient.h
we can see
TZmqClient override read and write methods
in test-client.cpp:
use TBinaryProtocol as the protocol
···
zmq::context_t ctx(1);
shared_ptr<TZmqClient> transport(new TZmqClient(ctx, endpoint, socktype));
shared_ptr<TBinaryProtocol> protocol(new TBinaryProtocol(transport));
StorageClient client(protocol);
transport->open();
if (incr) {
client.incr(incr);
usleep(50000);
} else {
int value = client.get();
std::cout << value << std::endl;
}
···
now client is a StorageClient object
client.incr callgraph :
StorageClient::incr-> StorageClient::send_incr -> TBinaryProtocol::WriteMessageBegin -> TBinaryProtocol::writeI32 -> TTransport::write
but TTransport::write is not a virtual method, it call TTransport::write_virt, this is the real virtual methods
in class TZmqClient, we override it cannot take effect
we need to override TTransport::write_virt
---
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.
---