> On Sept. 2, 2014, 7:35 p.m., Mark Chu-Carroll wrote:
> > src/main/python/apache/aurora/client/api/__init__.py, line 176
> > <https://reviews.apache.org/r/25204/diff/1/?file=672539#file672539line176>
> >
> >     Nit - but why are you changing the parameter comment syntax? We don't 
> > use the double-dash anywhere else in the client.

This is the style everything under client/api seems to follow. Just making it 
consistent.


> On Sept. 2, 2014, 7:35 p.m., Mark Chu-Carroll wrote:
> > src/main/python/apache/aurora/client/api/__init__.py, line 232
> > <https://reviews.apache.org/r/25204/diff/1/?file=672539#file672539line232>
> >
> >     I think this would be clearer inlined. Right now, it's pretty much an 
> > alternate name for the JobUpdateQuery constructor, but with different 
> > parameter names. It makes the code harder to follow, not easier.

The reason for this is mostly consistency (see build_query above) but I 
personally prefer this approach as it decouples query building from the thrift 
API call.


> On Sept. 2, 2014, 7:35 p.m., Mark Chu-Carroll wrote:
> > src/main/python/apache/aurora/client/api/__init__.py, line 252
> > <https://reviews.apache.org/r/25204/diff/1/?file=672539#file672539line252>
> >
> >     If we're going to the trouble of abstracting away the actual scheduler 
> > interface, I think we shouldn't be returning the raw Response datatype. 
> > This should return the updates, not the structure wrapping the updates in 
> > two layers of indirection.
> >     
> >     These kinds of methods should check the result code, raise an exception 
> > if the API call failed, and then return a meaningful, simple result. 
> >     
> >     (Same comment applies to get_job_update_details)

Make it 3 for consistency :) Nothing in that file attempts to return a result. 
Event the updater tries to fake a Response object in order to comply with 
general api pattern.


- Maxim


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25204/#review52064
-----------------------------------------------------------


On Aug. 29, 2014, 10:28 p.m., Maxim Khutornenko wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25204/
> -----------------------------------------------------------
> 
> (Updated Aug. 29, 2014, 10:28 p.m.)
> 
> 
> Review request for Aurora and Mark Chu-Carroll.
> 
> 
> Bugs: AURORA-615
>     https://issues.apache.org/jira/browse/AURORA-615
> 
> 
> Repository: aurora
> 
> 
> Description
> -------
> 
> Adding "get" job update client APIs.
> 
> 
> Diffs
> -----
> 
>   src/main/python/apache/aurora/client/api/__init__.py 
> 90462bf5920786ea1316c75a99c8382cf8c803a1 
>   src/test/python/apache/aurora/client/api/test_api.py 
> b47b6db31842fffba797c7f616b5f4deb8d04a86 
> 
> Diff: https://reviews.apache.org/r/25204/diff/
> 
> 
> Testing
> -------
> 
> ./pants src/test/python/apache/aurora/client/api:api -s
> 
> 
> Thanks,
> 
> Maxim Khutornenko
> 
>

Reply via email to