You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@thrift.apache.org by RobberPhex <gi...@git.apache.org> on 2016/05/31 17:21:20 UTC
[GitHub] thrift pull request: THRIFT-3845
GitHub user RobberPhex opened a pull request:
https://github.com/apache/thrift/pull/1022
THRIFT-3845
use `isBinaryAccelerated` to indicate use extension `thrift_protocol`.
When `TBinaryProtocolAccelerated` warped in `TMultiplexedProtocol`,extension `thrift_protocol` can also be used for `client serialization`, `client deserialization`, `server serialization`.
notice: `server deserialization` can't use extension `thrift_protocol`.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/RobberPhex/thrift THRIFT-3845
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/thrift/pull/1022.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 #1022
----
commit 30d3fb6000f84d401884a7799f2a96a08930e917
Author: Robert Lu <ro...@gmail.com>
Date: 2016-05-31T15:38:18Z
add isStrict{Read,Write} to TProtocolDecorator for TBinaryProtocolAccelerated
commit 2b9005a49c299ab444a21d61e38b1714f294f5cf
Author: Robert Lu <ro...@gmail.com>
Date: 2016-05-31T16:39:06Z
add isBinaryAccelerated for thrift_protocol ext
instanceof TBinaryProtocolAccelerated will caused
TBinaryProtocolAccelerated warped in TMultiplexedProtocol can't use
thrift_protocol extension
----
---
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 #1022: THRIFT-3845
Posted by RobberPhex <gi...@git.apache.org>.
Github user RobberPhex closed the pull request at:
https://github.com/apache/thrift/pull/1022
---
[GitHub] thrift pull request #1022: THRIFT-3845
Posted by nsuke <gi...@git.apache.org>.
Github user nsuke commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1022#discussion_r80386881
--- Diff: lib/php/lib/Thrift/Protocol/TBinaryProtocolAccelerated.php ---
@@ -62,4 +62,9 @@ public function isStrictWrite()
{
return $this->strictWrite_;
}
+
+ public function isBinaryAccelerated()
+ {
+ return true;
+ }
--- End diff --
I agree we need something like this but the name can be more general.
For example, `isAccelerated` or even `isDynamic`.
(Thinks about the day when we add TCompactProtocolAccelerated or anything without generated read/write calls...)
---
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 #1022: THRIFT-3845
Posted by nsuke <gi...@git.apache.org>.
Github user nsuke commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1022#discussion_r80386819
--- Diff: compiler/cpp/src/generate/t_php_generator.cc ---
@@ -1341,9 +1341,13 @@ void t_php_generator::generate_process_function(t_service* tservice, t_function*
return;
}
- f_service_ << indent() << "$bin_accel = ($output instanceof "
- << "TBinaryProtocolAccelerated) && function_exists('thrift_protocol_write_binary');"
+ // for compatibility, add method_exists
+ f_service_ << indent() << "$bin_accel = method_exists($output, 'isBinaryAccelerated')" << endl;
+ indent_up();
+ f_service_ << indent() << " && $output->isBinaryAccelerated()" << endl
+ << indent() << " && function_exists('thrift_protocol_write_binary');"
--- End diff --
The real problem may be that existing code generation here is too specific to one concrete type.
What if we move these thrift_protocol_..._binary calls to TBinaryProtocolAccelerated.php ?
That way we can move isStrict... checks there too so that we don't need them in TProtocolDecorator either.
---
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 #1022: THRIFT-3845
Posted by jeking3 <gi...@git.apache.org>.
Github user jeking3 commented on the issue:
https://github.com/apache/thrift/pull/1022
@RobberPhex could you rebase this against master so we can get a build that passes all tests?
---
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 #1022: THRIFT-3845
Posted by RobberPhex <gi...@git.apache.org>.
Github user RobberPhex commented on the issue:
https://github.com/apache/thrift/pull/1022
@jeking3 rebased
---
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.
---