You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@aurora.apache.org by Maxim Khutornenko <ma...@apache.org> on 2014/08/30 00:28:23 UTC
Review Request 25204: Adding "get" job update client APIs.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25204/
-----------------------------------------------------------
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
Re: Review Request 25204: Adding "get" job update client APIs.
Posted by Maxim Khutornenko <ma...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25204/
-----------------------------------------------------------
(Updated Sept. 3, 2014, 10:24 p.m.)
Review request for Aurora and Mark Chu-Carroll.
Changes
-------
CR comments.
Bugs: AURORA-615
https://issues.apache.org/jira/browse/AURORA-615
Repository: aurora
Description
-------
Adding "get" job update client APIs.
Diffs (updated)
-----
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
Re: Review Request 25204: Adding "get" job update client APIs.
Posted by Mark Chu-Carroll <mc...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25204/#review52214
-----------------------------------------------------------
Ship it!
Ship It!
- Mark Chu-Carroll
On Aug. 29, 2014, 6: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, 6: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
>
>
Re: Review Request 25204: Adding "get" job update client APIs.
Posted by Maxim Khutornenko <ma...@apache.org>.
> 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.
>
> Maxim Khutornenko wrote:
> 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.
>
> Mark Chu-Carroll wrote:
> In this case, I disagree pretty strongly - but if you must have the separate method, at least make the argument names match.
Feels like your disagreement is stronger than my preference in this case :). Changed.
- 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
>
>
Re: Review Request 25204: Adding "get" job update client APIs.
Posted by Maxim Khutornenko <ma...@apache.org>.
> 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
>
>
Re: Review Request 25204: Adding "get" job update client APIs.
Posted by Mark Chu-Carroll <mc...@twopensource.com>.
> On Sept. 2, 2014, 3: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.
>
> Maxim Khutornenko wrote:
> This is the style everything under client/api seems to follow. Just making it consistent.
Ick. So client/api doesn't follow the same pattern as a lot of other client code. Something for me to fix at some point.
> On Sept. 2, 2014, 3: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.
>
> Maxim Khutornenko wrote:
> 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.
In this case, I disagree pretty strongly - but if you must have the separate method, at least make the argument names match.
- Mark
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25204/#review52064
-----------------------------------------------------------
On Aug. 29, 2014, 6: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, 6: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
>
>
Re: Review Request 25204: Adding "get" job update client APIs.
Posted by Mark Chu-Carroll <mc...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25204/#review52064
-----------------------------------------------------------
src/main/python/apache/aurora/client/api/__init__.py
<https://reviews.apache.org/r/25204/#comment90799>
Nit - but why are you changing the parameter comment syntax? We don't use the double-dash anywhere else in the client.
src/main/python/apache/aurora/client/api/__init__.py
<https://reviews.apache.org/r/25204/#comment90801>
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.
src/main/python/apache/aurora/client/api/__init__.py
<https://reviews.apache.org/r/25204/#comment90802>
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)
- Mark Chu-Carroll
On Aug. 29, 2014, 6: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, 6: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
>
>