626633 - Optimization tip for Python Thrift communication

Status: RESOLVED FIXED

(Socorro :: General, task, P3)

Description

[Daniel Einspanjer [:dre] [:deinspanjer]]

Original mailing list link shared to us by Stack:
http://permalink.gmane.org/gmane.comp.java.hadoop.hbase.user/14379

Message contents:

Having worked with the other java/thrift based nosql solution we have been
using Thrift Accelerated Protocol and it works great. It is very fast and we
have seen 3-4x performance improvement on some read operations (wide rows).
We have never seen this "advertised" or referrenced with any hbase python
sample code so I thought I would post this to help.

from thrift.protocol.TBinaryProtocol import TBinaryProtocolAccelerated
...

protocol = TBinaryProtocolAccelerated(transport)
...

Comment 1

[Daniel Einspanjer [:dre] [:deinspanjer]]

Here is a link to the source docs on it:
http://code.google.com/p/socorro/source/browse/trunk/thirdparty/thrift/protocol/TBinaryProtocol.py#235

Comment 2

[Robert Helmer [:rhelmer]]

Checked this in, will test on staging momentarily:
Committed r2891

Comment 3

[Robert Helmer [:rhelmer]]

Attachment: use accelerated protocol

Not checked in yet but tested on staging, appears to work at first blush.

Lars - I am not sure why it uses "self.protocolModule.TBinaryProtocol(self.transport)" before this patch; can you enlighten me? Anything wrong with just doing "TBinaryProtocolAccelerated(self.transport)" here? 

I feel like I must be missing something...

Comment 4

[K Lars Lohn [:lars] [:klohn]]

Comment on attachment 505574 [details] [diff] [review]
use accelerated protocol

the new import pulls a class from a module while the previous import pulls a module from a package.  So they don't do equivalent actions.

then later, the original code uses self.protocolModule.TBufferedTransport to instantiate a class.  Your replacement bypasses the fetch the class from module step. By bypassing that step, we break the unit tests.  In the unit tests, we mock the protocol module and then to pull a mocked TBinaryProtocol from it.  The unit tests could be changed to work for your code.  However, the original code preserves the original authors methods (rightly or wrongly).

Comment 5

[Robert Helmer [:rhelmer]]

(In reply to comment #4)
> Comment on attachment 505574 [details] [diff] [review]
> use accelerated protocol
> 
> the new import pulls a class from a module while the previous import pulls a
> module from a package.  So they don't do equivalent actions.
> 
> then later, the original code uses self.protocolModule.TBufferedTransport to
> instantiate a class.  Your replacement bypasses the fetch the class from module
> step. By bypassing that step, we break the unit tests.  In the unit tests, we
> mock the protocol module and then to pull a mocked TBinaryProtocol from it. 
> The unit tests could be changed to work for your code.  However, the original
> code preserves the original authors methods (rightly or wrongly).

Ah thanks, I see it now. 

I backed this out for now, since we decided not to move forward with this today:
Committed r2891

I'll post a new patch later, and fix the unit test too.

Comment 6

[Robert Helmer [:rhelmer]]

Attachment: use accelerated protocol

use thrift.protocol.TBinaryProtocol.TBinaryProtocolAccerated class without breaking unit tests (not to be confused with thrift.protocol.TBinaryProtocol.TBinaryProtocol class :).

Tested using hbaseClient.py, but I know that it just fell back from looking at the "implementation" (and very helpful comment) in the thrift-generated python code, we'll need to build the C bindings and make the module available for this to work:

class TBinaryProtocolAccelerated(TBinaryProtocol):

  """C-Accelerated version of TBinaryProtocol.

  This class does not override any of TBinaryProtocol's methods,
  but the generated code recognizes it directly and will call into
  our C module to do the encoding, bypassing this object entirely.
  We inherit from TBinaryProtocol so that the normal TBinaryProtocol
  encoding can happen if the fastbinary module doesn't work for some
  reason.  (TODO(dreiss): Make this happen sanely in more cases.)

  In order to take advantage of the C module, just use
  TBinaryProtocolAccelerated instead of TBinaryProtocol.

  NOTE:  This code was contributed by an external developer.
         The internal Thrift team has reviewed and tested it,
         but we cannot guarantee that it is production-ready.
         Please feel free to report bugs and/or success stories
         to the public mailing list.
  """

  pass

I made a small local modification to make sure that this class is being called from HBase, that looks fine at least.

I'll try to get this working in dev, this patch should be safe to take in the meantime since it'll just fall back if the module is not available.

Comment 7

[K Lars Lohn [:lars] [:klohn]]

Comment on attachment 508085 [details] [diff] [review]
use accelerated protocol

I recall looking at this quite a while ago and thinking it looked fine.

Comment 8

[Daniel Einspanjer [:dre] [:deinspanjer]]

Note: to get the binary component installed on a different machine, I did the following:

wget http://pypi.python.org/packages/source/t/thrift/Thrift-0.5.0.tar.gz
tar xzvf Thrift-0.5.0.tar.gz
rm Thrift-0.5.0.tar.gz 
mv Thrift-0.5.0 src/
cd src/Thrift-0.5.0/
yum install python26-devel
python26 setup.py build
python26 setup.py build
python26 setup.py install

Comment 9

[Robert Helmer [:rhelmer]]

(In reply to comment #8)
> Note: to get the binary component installed on a different machine, I did the
> following:
> 
> wget http://pypi.python.org/packages/source/t/thrift/Thrift-0.5.0.tar.gz
> tar xzvf Thrift-0.5.0.tar.gz
> rm Thrift-0.5.0.tar.gz 
> mv Thrift-0.5.0 src/
> cd src/Thrift-0.5.0/
> yum install python26-devel
> python26 setup.py build
> python26 setup.py build
> python26 setup.py install

Oh cool, didn't realize it was on pypi, that makes it a lot easier.

In fact, if I just do "pip install thrift" in my virtualenv, I get the fastbinary.so, so should be all set. We should be able to trivially build an RPM from the pypi package too.

I'll do some tests with my dev socorro against the research cluster.

Comment 10

[Robert Helmer [:rhelmer]]

Seems ok from initial testing. Now that new stage is up, would love to test for perf/correctness.

Comment 11

[Robert Helmer [:rhelmer]]

Not working on this right now, but would be easy to do.

Comment 12

[Robert Helmer [:rhelmer]]

This is fixed by bug 751654, and in production as of Socorro 9.