Add a twisted log client

Add a twisted log client

[ekasper, 2013-12-19 16:05:49]

Hi Al,

This is my Christmas present to you, I hope you like it!

It's a little asynchronous twisted client for talking to the log. I have no idea
yet what this may be good for, and we may never find out but I thought you might
enjoy reviewing it anyway. The client currently only implements get_sth() to
keep the change manageable. Here's a little twisted dictionary to aid the
review:

reactor - selectserver
Deferred - Future
Failure - Exception
callback - callback

Merry Christmas!

Emilia

[ekasper, 2014-01-10 14:04:16]

I'm not in a hurry but just in case this fell through the cracks of the
holiday season... bump!


On Thu, Dec 19, 2013 at 5:05 PM, <ekasper@google.com> wrote:

> Reviewers: Al Cutter,
>
> Message:
> Hi Al,
>
> This is my Christmas present to you, I hope you like it!
>
> It's a little asynchronous twisted client for talking to the log. I have
> no idea yet what this may be good for, and we may never find out but I
> thought you might enjoy reviewing it anyway. The client currently only
> implements get_sth() to keep the change manageable. Here's a little
> twisted dictionary to aid the review:
>
> reactor - selectserver
> Deferred - Future
> Failure - Exception
> callback - callback
>
> Merry Christmas!
>
> Emilia
>
>
>
>
> Description:
> Add a twisted log client
>
> Please review this at https://codereview.appspot.com/43650050/
>
> Affected files (+626, -283 lines):
>   M src/python/Makefile
>   M src/python/README
>   A src/python/ct/client/async_log_client_test.py
>   M src/python/ct/client/log_client.py
>   M src/python/ct/client/log_client_test.py
>   A src/python/ct/client/log_client_test_util.py
>   A src/python/ct/client/tools/async_client_example.py
>
>
>

[Al Cutter, 2014-01-13 15:02:35]

lgtm (merry christmas!)

[Eran, 2014-01-16 14:09:48]

Drive-by LGTM and some minor comments.

https://codereview.appspot.com/43650050/diff/80001/src/python/ct/client/async...
File src/python/ct/client/async_log_client_test.py (right):

https://codereview.appspot.com/43650050/diff/80001/src/python/ct/client/async...
src/python/ct/client/async_log_client_test.py:23: transport =
proto_helpers.StringTransportWithDisconnection()
This initialization repeats over all three test cases, is there a way to
initialize finished, handler and transport in a setUp method with Twisted's
unittests?

https://codereview.appspot.com/43650050/diff/80001/src/python/ct/client/async...
src/python/ct/client/async_log_client_test.py:58: # TODO(ekasper): find a more
elegant and robust way to save flags.
Such as a parameter to the c'tor?

https://codereview.appspot.com/43650050/diff/80001/src/python/ct/client/log_c...
File src/python/ct/client/log_client.py (right):

https://codereview.appspot.com/43650050/diff/80001/src/python/ct/client/log_c...
src/python/ct/client/log_client.py:102: class RequestHandler(object):
Just curious, why not mock out the 'requests' package directly? 
Other than translating error codes, that class does not seem like it does much.
If we want to decouple our code from 'requests', that'd make sense.

https://codereview.appspot.com/43650050/diff/80001/src/python/ct/client/log_c...
src/python/ct/client/log_client.py:358: response =
self._req_body(_GET_PROOF_BY_HASH_PATH,
Why not inlive:
response = json.loads(self._req_body(...))

?
The intermediate unparsed string value is not really used.

https://codereview.appspot.com/43650050/diff/80001/src/python/ct/client/log_c...
src/python/ct/client/log_client.py:475: self._buffer.append(data)
nit: self._buffer.extend(data) so that later on you won't need to join all
pieces in the buffer.

[ekasper, 2014-01-20 15:19:58]

Eran, I've already submitted this but I'd put together a follow-up. Could you
clarify a few of your comments first, though? (No comment = I agree.)

https://codereview.appspot.com/43650050/diff/80001/src/python/ct/client/async...
File src/python/ct/client/async_log_client_test.py (right):

https://codereview.appspot.com/43650050/diff/80001/src/python/ct/client/async...
src/python/ct/client/async_log_client_test.py:58: # TODO(ekasper): find a more
elegant and robust way to save flags.
On 2014/01/16 14:09:49, Eran wrote:
> Such as a parameter to the c'tor?

I'm afraid I don't follow you here: could you spell it out for me?

https://codereview.appspot.com/43650050/diff/80001/src/python/ct/client/log_c...
File src/python/ct/client/log_client.py (right):

https://codereview.appspot.com/43650050/diff/80001/src/python/ct/client/log_c...
src/python/ct/client/log_client.py:102: class RequestHandler(object):
On 2014/01/16 14:09:49, Eran wrote:
> Just curious, why not mock out the 'requests' package directly? 
> Other than translating error codes, that class does not seem like it does
much.
> If we want to decouple our code from 'requests', that'd make sense.

I'm not sure what the pythonic way is here but... if we want to decouple the
code from 'requests', doesn't it make _more_ sense to use a wrapper class?

Right now I could at least theoretically change the handler to use another
module underneath without modifying the tests. But if I mock (using mock.patch?)
the module directly then the unit tests will have to become aware of requests.

https://codereview.appspot.com/43650050/diff/80001/src/python/ct/client/log_c...
src/python/ct/client/log_client.py:475: self._buffer.append(data)
On 2014/01/16 14:09:49, Eran wrote:
> nit: self._buffer.extend(data) so that later on you won't need to join all
> pieces in the buffer.

How does .extend() save me from eventually joining the list?

[Eran, 2014-02-04 11:59:18]

Clarified.

https://codereview.appspot.com/43650050/diff/80001/src/python/ct/client/async...
File src/python/ct/client/async_log_client_test.py (right):

https://codereview.appspot.com/43650050/diff/80001/src/python/ct/client/async...
src/python/ct/client/async_log_client_test.py:58: # TODO(ekasper): find a more
elegant and robust way to save flags.
On 2014/01/20 15:20:00, ekasper wrote:
> On 2014/01/16 14:09:49, Eran wrote:
> > Such as a parameter to the c'tor?
> 
> I'm afraid I don't follow you here: could you spell it out for me?

Simply pass in the value of FLAGS.response_buffer_size_bytes into the
ResponseBodyHandler constructor, rather than save and restore flags for tests.

https://codereview.appspot.com/43650050/diff/80001/src/python/ct/client/log_c...
File src/python/ct/client/log_client.py (right):

https://codereview.appspot.com/43650050/diff/80001/src/python/ct/client/log_c...
src/python/ct/client/log_client.py:102: class RequestHandler(object):
On 2014/01/20 15:20:00, ekasper wrote:
> On 2014/01/16 14:09:49, Eran wrote:
> > Just curious, why not mock out the 'requests' package directly? 
> > Other than translating error codes, that class does not seem like it does
> much.
> > If we want to decouple our code from 'requests', that'd make sense.
> 
> I'm not sure what the pythonic way is here but... if we want to decouple the
> code from 'requests', doesn't it make _more_ sense to use a wrapper class?
> 
> Right now I could at least theoretically change the handler to use another
> module underneath without modifying the tests. But if I mock (using
mock.patch?)
> the module directly then the unit tests will have to become aware of requests.
> 

Sorry, should have been clearer: If the goal of this wrapper class is to ease
testing, then I think using mock.patch to mock 'requests' module is perfectly
fine. I don't see an issue with the tests knowing about the implementation's use
of 'requests'.

https://codereview.appspot.com/43650050/diff/80001/src/python/ct/client/log_c...
src/python/ct/client/log_client.py:475: self._buffer.append(data)
On 2014/01/20 15:20:00, ekasper wrote:
> On 2014/01/16 14:09:49, Eran wrote:
> > nit: self._buffer.extend(data) so that later on you won't need to join all
> > pieces in the buffer.
> 
> How does .extend() save me from eventually joining the list?
> 

Sorry, meant concatenating into a string. Actually, this is fine  - more
efficient to create the string once.