You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@thrift.apache.org by "David Reiss (JIRA)" <ji...@apache.org> on 2008/11/21 01:58:44 UTC
[jira] Created: (THRIFT-212) Accelerated Binary Protocol support
for TFramedTransport
Accelerated Binary Protocol support for TFramedTransport
--------------------------------------------------------
Key: THRIFT-212
URL: https://issues.apache.org/jira/browse/THRIFT-212
Project: Thrift
Issue Type: Improvement
Components: Library (Python)
Reporter: David Reiss
Priority: Minor
This change makes TFramedTransport implement the CReadableTransport interface so TBinaryProtocolAccelerated can use the fastbinary C module to read from it.
Patches: http://gitweb.thrift-rpc.org/?p=thrift.git;a=log;h=refs/heads/pri/dreiss/py-cread-framed;hb=HEAD
This change was discussed earlier this year: http://publists.facebook.com/pipermail/thrift/2008-March/000735.html
bq. Could you change this to avoid the concat? You'd simply have to offset the initial buffer by 4 bytes and then do:
This is orthogonal to the CReadable change, so it doesn't belong in this issue.
bq. It'd be helpful to have a unit test for that case to prevent accidential optimization.
Again, orthogonal. I only touched this part of the code in a very superficial way.
bq. In the case where partialread == "" (which is the norm for the framed transport) this results in an extra copy.
This is not the case. I took a look at the Python source (the latest svn trunk), and it optimizes the case of concatenating with an empty string.
bq. Imagine there was a string field fragmented across multiple packets.
I fixed this issue and added a test for it.
bq. I'd really appreciate a unit test on this stuff
The TNonblockingServer test and the split frame test exercise it pretty well.
bq. a transport that makes read() return a random number of bytes each time.
It wouldn't make sense to put such a transport between the TBinaryProtocolAccelerated and the TFramedTransport. Putting it under the TFramedTransport would have no effect since TFramedTransport only uses readAll to get data from the underlying transport.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (THRIFT-212) Accelerated Binary Protocol support
for TFramedTransport
Posted by "Ben Maurer (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-212?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12669257#action_12669257 ]
Ben Maurer commented on THRIFT-212:
-----------------------------------
All of the patches LGTM.
> Accelerated Binary Protocol support for TFramedTransport
> --------------------------------------------------------
>
> Key: THRIFT-212
> URL: https://issues.apache.org/jira/browse/THRIFT-212
> Project: Thrift
> Issue Type: Improvement
> Components: Library (Python)
> Reporter: David Reiss
> Assignee: David Reiss
> Priority: Minor
> Attachments: 0001-python-Make-the-unit-tests-run-faster-and-more-reli.patch, 0002-cpp-Make-the-test-server-compatible-with-the-Python.patch, 0003-python-Eliminate-the-option-of-TFramedTransport-to.patch, 0004-python-Make-TFramedTransport-use-a-cStringIO-for-re.patch, 0005-THRIFT-212.-python-Make-TFramedTransport-implement.patch
>
>
> This change makes TFramedTransport implement the CReadableTransport interface so TBinaryProtocolAccelerated can use the fastbinary C module to read from it.
> Patches: http://gitweb.thrift-rpc.org/?p=thrift.git;a=log;h=refs/heads/pri/dreiss/py-cread-framed;hb=HEAD
> This change was discussed earlier this year: http://publists.facebook.com/pipermail/thrift/2008-March/000735.html
> bq. Could you change this to avoid the concat? You'd simply have to offset the initial buffer by 4 bytes and then do:
> This is orthogonal to the CReadable change, so it doesn't belong in this issue.
> bq. It'd be helpful to have a unit test for that case to prevent accidential optimization.
> Again, orthogonal. I only touched this part of the code in a very superficial way.
> bq. In the case where partialread == "" (which is the norm for the framed transport) this results in an extra copy.
> This is not the case. I took a look at the Python source (the latest svn trunk), and it optimizes the case of concatenating with an empty string.
> bq. Imagine there was a string field fragmented across multiple packets.
> I fixed this issue and added a test for it.
> bq. I'd really appreciate a unit test on this stuff
> The TNonblockingServer test and the split frame test exercise it pretty well.
> bq. a transport that makes read() return a random number of bytes each time.
> It wouldn't make sense to put such a transport between the TBinaryProtocolAccelerated and the TFramedTransport. Putting it under the TFramedTransport would have no effect since TFramedTransport only uses readAll to get data from the underlying transport.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Resolved: (THRIFT-212) Accelerated Binary Protocol support
for TFramedTransport
Posted by "David Reiss (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-212?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
David Reiss resolved THRIFT-212.
--------------------------------
Resolution: Fixed
> Accelerated Binary Protocol support for TFramedTransport
> --------------------------------------------------------
>
> Key: THRIFT-212
> URL: https://issues.apache.org/jira/browse/THRIFT-212
> Project: Thrift
> Issue Type: Improvement
> Components: Library (Python)
> Reporter: David Reiss
> Assignee: David Reiss
> Priority: Minor
> Attachments: 0001-python-Make-the-unit-tests-run-faster-and-more-reli.patch, 0002-cpp-Make-the-test-server-compatible-with-the-Python.patch, 0003-python-Eliminate-the-option-of-TFramedTransport-to.patch, 0004-python-Make-TFramedTransport-use-a-cStringIO-for-re.patch, 0005-THRIFT-212.-python-Make-TFramedTransport-implement.patch
>
>
> This change makes TFramedTransport implement the CReadableTransport interface so TBinaryProtocolAccelerated can use the fastbinary C module to read from it.
> Patches: http://gitweb.thrift-rpc.org/?p=thrift.git;a=log;h=refs/heads/pri/dreiss/py-cread-framed;hb=HEAD
> This change was discussed earlier this year: http://publists.facebook.com/pipermail/thrift/2008-March/000735.html
> bq. Could you change this to avoid the concat? You'd simply have to offset the initial buffer by 4 bytes and then do:
> This is orthogonal to the CReadable change, so it doesn't belong in this issue.
> bq. It'd be helpful to have a unit test for that case to prevent accidential optimization.
> Again, orthogonal. I only touched this part of the code in a very superficial way.
> bq. In the case where partialread == "" (which is the norm for the framed transport) this results in an extra copy.
> This is not the case. I took a look at the Python source (the latest svn trunk), and it optimizes the case of concatenating with an empty string.
> bq. Imagine there was a string field fragmented across multiple packets.
> I fixed this issue and added a test for it.
> bq. I'd really appreciate a unit test on this stuff
> The TNonblockingServer test and the split frame test exercise it pretty well.
> bq. a transport that makes read() return a random number of bytes each time.
> It wouldn't make sense to put such a transport between the TBinaryProtocolAccelerated and the TFramedTransport. Putting it under the TFramedTransport would have no effect since TFramedTransport only uses readAll to get data from the underlying transport.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Assigned: (THRIFT-212) Accelerated Binary Protocol support
for TFramedTransport
Posted by "David Reiss (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-212?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
David Reiss reassigned THRIFT-212:
----------------------------------
Assignee: David Reiss
> Accelerated Binary Protocol support for TFramedTransport
> --------------------------------------------------------
>
> Key: THRIFT-212
> URL: https://issues.apache.org/jira/browse/THRIFT-212
> Project: Thrift
> Issue Type: Improvement
> Components: Library (Python)
> Reporter: David Reiss
> Assignee: David Reiss
> Priority: Minor
>
> This change makes TFramedTransport implement the CReadableTransport interface so TBinaryProtocolAccelerated can use the fastbinary C module to read from it.
> Patches: http://gitweb.thrift-rpc.org/?p=thrift.git;a=log;h=refs/heads/pri/dreiss/py-cread-framed;hb=HEAD
> This change was discussed earlier this year: http://publists.facebook.com/pipermail/thrift/2008-March/000735.html
> bq. Could you change this to avoid the concat? You'd simply have to offset the initial buffer by 4 bytes and then do:
> This is orthogonal to the CReadable change, so it doesn't belong in this issue.
> bq. It'd be helpful to have a unit test for that case to prevent accidential optimization.
> Again, orthogonal. I only touched this part of the code in a very superficial way.
> bq. In the case where partialread == "" (which is the norm for the framed transport) this results in an extra copy.
> This is not the case. I took a look at the Python source (the latest svn trunk), and it optimizes the case of concatenating with an empty string.
> bq. Imagine there was a string field fragmented across multiple packets.
> I fixed this issue and added a test for it.
> bq. I'd really appreciate a unit test on this stuff
> The TNonblockingServer test and the split frame test exercise it pretty well.
> bq. a transport that makes read() return a random number of bytes each time.
> It wouldn't make sense to put such a transport between the TBinaryProtocolAccelerated and the TFramedTransport. Putting it under the TFramedTransport would have no effect since TFramedTransport only uses readAll to get data from the underlying transport.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Updated: (THRIFT-212) Accelerated Binary Protocol support
for TFramedTransport
Posted by "David Reiss (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-212?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
David Reiss updated THRIFT-212:
-------------------------------
Attachment: 0005-THRIFT-212.-python-Make-TFramedTransport-implement.patch
0004-python-Make-TFramedTransport-use-a-cStringIO-for-re.patch
0003-python-Eliminate-the-option-of-TFramedTransport-to.patch
0002-cpp-Make-the-test-server-compatible-with-the-Python.patch
0001-python-Make-the-unit-tests-run-faster-and-more-reli.patch
> Accelerated Binary Protocol support for TFramedTransport
> --------------------------------------------------------
>
> Key: THRIFT-212
> URL: https://issues.apache.org/jira/browse/THRIFT-212
> Project: Thrift
> Issue Type: Improvement
> Components: Library (Python)
> Reporter: David Reiss
> Assignee: David Reiss
> Priority: Minor
> Attachments: 0001-python-Make-the-unit-tests-run-faster-and-more-reli.patch, 0002-cpp-Make-the-test-server-compatible-with-the-Python.patch, 0003-python-Eliminate-the-option-of-TFramedTransport-to.patch, 0004-python-Make-TFramedTransport-use-a-cStringIO-for-re.patch, 0005-THRIFT-212.-python-Make-TFramedTransport-implement.patch
>
>
> This change makes TFramedTransport implement the CReadableTransport interface so TBinaryProtocolAccelerated can use the fastbinary C module to read from it.
> Patches: http://gitweb.thrift-rpc.org/?p=thrift.git;a=log;h=refs/heads/pri/dreiss/py-cread-framed;hb=HEAD
> This change was discussed earlier this year: http://publists.facebook.com/pipermail/thrift/2008-March/000735.html
> bq. Could you change this to avoid the concat? You'd simply have to offset the initial buffer by 4 bytes and then do:
> This is orthogonal to the CReadable change, so it doesn't belong in this issue.
> bq. It'd be helpful to have a unit test for that case to prevent accidential optimization.
> Again, orthogonal. I only touched this part of the code in a very superficial way.
> bq. In the case where partialread == "" (which is the norm for the framed transport) this results in an extra copy.
> This is not the case. I took a look at the Python source (the latest svn trunk), and it optimizes the case of concatenating with an empty string.
> bq. Imagine there was a string field fragmented across multiple packets.
> I fixed this issue and added a test for it.
> bq. I'd really appreciate a unit test on this stuff
> The TNonblockingServer test and the split frame test exercise it pretty well.
> bq. a transport that makes read() return a random number of bytes each time.
> It wouldn't make sense to put such a transport between the TBinaryProtocolAccelerated and the TFramedTransport. Putting it under the TFramedTransport would have no effect since TFramedTransport only uses readAll to get data from the underlying transport.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.