thriftcli icon indicating copy to clipboard operation
thriftcli copied to clipboard

Add Thrift HTTPClient Support

Open michaeljs1990 opened this issue 7 years ago • 6 comments

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```

michaeljs1990 avatar Apr 17 '18 20:04 michaeljs1990

Coverage Status

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

coveralls avatar Apr 17 '18 20:04 coveralls

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

michaeljs1990 avatar Apr 17 '18 21:04 michaeljs1990

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

michaeljs1990 avatar Oct 08 '18 07:10 michaeljs1990

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

vtatai avatar Jan 29 '19 00:01 vtatai

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

vtatai avatar Feb 06 '19 22:02 vtatai

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.

michaeljs1990 avatar Feb 07 '19 04:02 michaeljs1990