Skip to content
This repository has been archived by the owner on Feb 25, 2023. It is now read-only.

Add Thrift HTTPClient Support #15

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

michaeljs1990
Copy link

@michaeljs1990 michaeljs1990 commented Apr 17, 2018

This adds support for sending requests to thrift when the message must go over HTTP instead of the default thrift transport.

Another flag should be made to allow to set the path however I'm curious on thoughts around how this should be done if upstream is interested in supporting it. If so we could have a --http_client_path flag and doing that defaults to / and turns on the http client so we don't need two flags to support the http client.

Tested locally as well using

# Using new -hc flag
$ python setup.py install && ./thrift/bin/thriftcli -hc service.app.terame.net:80 Service.health_check test_thrift
HealthCheckReply(healthy=True)
# Using default thrift transport fails as expected
$ ./thrift/bin/thriftcli service.app.terame.net:80 Service.health_check test_thrift
Traceback (most recent call last):
  File "./thrift/bin/thriftcli", line 11, in <module>
    load_entry_point('thriftcli==1.1', 'console_scripts', 'thriftcli')()
  File "build/bdist.macosx-10.12-x86_64/egg/thriftcli/thrift_cli.py", line 287, in main
  File "build/bdist.macosx-10.12-x86_64/egg/thriftcli/thrift_cli.py", line 257, in _run_cli
  File "build/bdist.macosx-10.12-x86_64/egg/thriftcli/thrift_cli.py", line 57, in __init__
  File "build/bdist.macosx-10.12-x86_64/egg/thriftcli/thrift_executor.py", line 55, in __init__
  File "build/bdist.macosx-10.12-x86_64/egg/thriftcli/thrift_executor.py", line 143, in _open_connection
  File "build/bdist.macosx-10.12-x86_64/egg/twitter/common/rpc/finagle/protocol.py", line 70, in __init__
  File "build/bdist.macosx-10.12-x86_64/egg/twitter/common/rpc/finagle/protocol.py", line 59, in upgrade_protocol_to_finagle
  File "build/bdist.macosx-10.12-x86_64/egg/twitter/common/rpc/finagle/protocol.py", line 45, in recv
  File "build/bdist.macosx-10.12-x86_64/egg/twitter/common/rpc/finagle/protocol.py", line 98, in readMessageBegin
  File "/Users/schuettm/Code/thriftcli/thrift/lib/python2.7/site-packages/thrift-0.11.0-py2.7-macosx-10.12-x86_64.egg/thrift/protocol/TBinaryProtocol.py", line 134, in readMessageBegin
    sz = self.readI32()
  File "/Users/schuettm/Code/thriftcli/thrift/lib/python2.7/site-packages/thrift-0.11.0-py2.7-macosx-10.12-x86_64.egg/thrift/protocol/TBinaryProtocol.py", line 217, in readI32
    buff = self.trans.readAll(4)
  File "/Users/schuettm/Code/thriftcli/thrift/lib/python2.7/site-packages/thrift-0.11.0-py2.7-macosx-10.12-x86_64.egg/thrift/transport/TTransport.py", line 60, in readAll
    chunk = self.read(sz - have)
  File "/Users/schuettm/Code/thriftcli/thrift/lib/python2.7/site-packages/thrift-0.11.0-py2.7-macosx-10.12-x86_64.egg/thrift/transport/TTransport.py", line 281, in read
    self.readFrame()
  File "/Users/schuettm/Code/thriftcli/thrift/lib/python2.7/site-packages/thrift-0.11.0-py2.7-macosx-10.12-x86_64.egg/thrift/transport/TTransport.py", line 287, in readFrame
    self.__rbuf = BufferIO(self.__trans.readAll(sz))
  File "/Users/schuettm/Code/thriftcli/thrift/lib/python2.7/site-packages/thrift-0.11.0-py2.7-macosx-10.12-x86_64.egg/thrift/transport/TTransport.py", line 60, in readAll
    chunk = self.read(sz - have)
  File "/Users/schuettm/Code/thriftcli/thrift/lib/python2.7/site-packages/thrift-0.11.0-py2.7-macosx-10.12-x86_64.egg/thrift/transport/TSocket.py", line 132, in read
    message='TSocket read 0 bytes')
thrift.transport.TTransport.TTransportException: TSocket read 0 bytes```

This adds support for sending requests to thrift when the message must
go over HTTP instead of the default thrift transport.
Should add some better checking for this however I'll wait to make
sure that upstream is OK before adding too much.
@coveralls
Copy link

coveralls commented Apr 17, 2018

Coverage Status

Coverage decreased (-0.04%) to 85.779% when pulling b2e18ae on michaeljs1990:master into 6fbdd5d on Fitbit:master.

@michaeljs1990
Copy link
Author

Alright i'll dig around to get this coverage up but don't see a real clean path.

@michaeljs1990
Copy link
Author

hello, just a ping if someone has time to take a look.

@vtatai
Copy link
Contributor

vtatai commented Jan 29, 2019

@michaeljs1990 sorry - trying to get back on the horse. I'll take a look at it.

@vtatai
Copy link
Contributor

vtatai commented Feb 6, 2019

@michaeljs1990 this seems straightfwd enough - are you still interested in having it merged?

@michaeljs1990
Copy link
Author

I personally don't need this code landed anymore but if you see it as generally useful for others it would be cool to land it.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants