[
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.
