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/14 18:08:09 UTC
Review Request 24702: Implementing client job lock and start update
APIs.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24702/
-----------------------------------------------------------
Review request for Aurora, Mark Chu-Carroll and Bill Farner.
Bugs: AURORA-615
https://issues.apache.org/jira/browse/AURORA-615
Repository: aurora
Description
-------
Implementing client job lock and start update APIs.
Diffs
-----
src/main/python/apache/aurora/client/api/__init__.py 371137e469fd65935f4048eaafdcf9b702e8c947
src/main/python/apache/aurora/client/api/updater_util.py cbb93e4d64a334dba9d091b68cf6d3594930fa31
src/test/python/apache/aurora/client/api/BUILD db5c22375378566f58ade5d6a0b7fcce75ad8394
src/test/python/apache/aurora/client/api/test_api.py PRE-CREATION
Diff: https://reviews.apache.org/r/24702/diff/
Testing
-------
./pants src/test/python/apache/aurora/client/api:api
Thanks,
Maxim Khutornenko
Re: Review Request 24702: Implementing client job lock and start
update APIs.
Posted by Maxim Khutornenko <ma...@apache.org>.
> On Aug. 14, 2014, 5:24 p.m., Bill Farner wrote:
> > src/main/python/apache/aurora/client/api/updater_util.py, line 68
> > <https://reviews.apache.org/r/24702/diff/1/?file=660625#file660625line68>
> >
> > The doc here might also help explain the behavior. This wording would be more obvious: "Groups instance IDs into a set of contiguous integer ranges."
Done.
- Maxim
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24702/#review50599
-----------------------------------------------------------
On Aug. 14, 2014, 4:08 p.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24702/
> -----------------------------------------------------------
>
> (Updated Aug. 14, 2014, 4:08 p.m.)
>
>
> Review request for Aurora, Mark Chu-Carroll and Bill Farner.
>
>
> Bugs: AURORA-615
> https://issues.apache.org/jira/browse/AURORA-615
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Implementing client job lock and start update APIs.
>
>
> Diffs
> -----
>
> src/main/python/apache/aurora/client/api/__init__.py 371137e469fd65935f4048eaafdcf9b702e8c947
> src/main/python/apache/aurora/client/api/updater_util.py cbb93e4d64a334dba9d091b68cf6d3594930fa31
> src/test/python/apache/aurora/client/api/BUILD db5c22375378566f58ade5d6a0b7fcce75ad8394
> src/test/python/apache/aurora/client/api/test_api.py PRE-CREATION
>
> Diff: https://reviews.apache.org/r/24702/diff/
>
>
> Testing
> -------
>
> ./pants src/test/python/apache/aurora/client/api:api
>
>
> Thanks,
>
> Maxim Khutornenko
>
>
Re: Review Request 24702: Implementing client job lock and start
update APIs.
Posted by Bill Farner <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24702/#review50599
-----------------------------------------------------------
src/main/python/apache/aurora/client/api/updater_util.py
<https://reviews.apache.org/r/24702/#comment88469>
The doc here might also help explain the behavior. This wording would be more obvious: "Groups instance IDs into a set of contiguous integer ranges."
- Bill Farner
On Aug. 14, 2014, 4:08 p.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24702/
> -----------------------------------------------------------
>
> (Updated Aug. 14, 2014, 4:08 p.m.)
>
>
> Review request for Aurora, Mark Chu-Carroll and Bill Farner.
>
>
> Bugs: AURORA-615
> https://issues.apache.org/jira/browse/AURORA-615
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Implementing client job lock and start update APIs.
>
>
> Diffs
> -----
>
> src/main/python/apache/aurora/client/api/__init__.py 371137e469fd65935f4048eaafdcf9b702e8c947
> src/main/python/apache/aurora/client/api/updater_util.py cbb93e4d64a334dba9d091b68cf6d3594930fa31
> src/test/python/apache/aurora/client/api/BUILD db5c22375378566f58ade5d6a0b7fcce75ad8394
> src/test/python/apache/aurora/client/api/test_api.py PRE-CREATION
>
> Diff: https://reviews.apache.org/r/24702/diff/
>
>
> Testing
> -------
>
> ./pants src/test/python/apache/aurora/client/api:api
>
>
> Thanks,
>
> Maxim Khutornenko
>
>
Re: Review Request 24702: Implementing client job lock and start
update APIs.
Posted by Maxim Khutornenko <ma...@apache.org>.
> On Aug. 14, 2014, 5:21 p.m., Bill Farner wrote:
> > src/main/python/apache/aurora/client/api/__init__.py, line 158
> > <https://reviews.apache.org/r/24702/diff/1/?file=660624#file660624line158>
> >
> > previously-acquired
Changed.
- Maxim
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24702/#review50598
-----------------------------------------------------------
On Aug. 14, 2014, 4:08 p.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24702/
> -----------------------------------------------------------
>
> (Updated Aug. 14, 2014, 4:08 p.m.)
>
>
> Review request for Aurora, Mark Chu-Carroll and Bill Farner.
>
>
> Bugs: AURORA-615
> https://issues.apache.org/jira/browse/AURORA-615
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Implementing client job lock and start update APIs.
>
>
> Diffs
> -----
>
> src/main/python/apache/aurora/client/api/__init__.py 371137e469fd65935f4048eaafdcf9b702e8c947
> src/main/python/apache/aurora/client/api/updater_util.py cbb93e4d64a334dba9d091b68cf6d3594930fa31
> src/test/python/apache/aurora/client/api/BUILD db5c22375378566f58ade5d6a0b7fcce75ad8394
> src/test/python/apache/aurora/client/api/test_api.py PRE-CREATION
>
> Diff: https://reviews.apache.org/r/24702/diff/
>
>
> Testing
> -------
>
> ./pants src/test/python/apache/aurora/client/api:api
>
>
> Thanks,
>
> Maxim Khutornenko
>
>
Re: Review Request 24702: Implementing client job lock and start
update APIs.
Posted by Maxim Khutornenko <ma...@apache.org>.
> On Aug. 14, 2014, 5:21 p.m., Bill Farner wrote:
> > src/main/python/apache/aurora/client/api/__init__.py, line 181
> > <https://reviews.apache.org/r/24702/diff/1/?file=660624#file660624line181>
> >
> > Seems like the scheduler should automatically do this within startJobUpdate. Is there anything preventing that behavior?
That's actually a great idea! The startJobUpdate already does ConfigurationManager.populateAndValidate() on the TaskConfig making this call effectively redundant. Will drop it entirely.
- Maxim
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24702/#review50598
-----------------------------------------------------------
On Aug. 14, 2014, 4:08 p.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24702/
> -----------------------------------------------------------
>
> (Updated Aug. 14, 2014, 4:08 p.m.)
>
>
> Review request for Aurora, Mark Chu-Carroll and Bill Farner.
>
>
> Bugs: AURORA-615
> https://issues.apache.org/jira/browse/AURORA-615
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Implementing client job lock and start update APIs.
>
>
> Diffs
> -----
>
> src/main/python/apache/aurora/client/api/__init__.py 371137e469fd65935f4048eaafdcf9b702e8c947
> src/main/python/apache/aurora/client/api/updater_util.py cbb93e4d64a334dba9d091b68cf6d3594930fa31
> src/test/python/apache/aurora/client/api/BUILD db5c22375378566f58ade5d6a0b7fcce75ad8394
> src/test/python/apache/aurora/client/api/test_api.py PRE-CREATION
>
> Diff: https://reviews.apache.org/r/24702/diff/
>
>
> Testing
> -------
>
> ./pants src/test/python/apache/aurora/client/api:api
>
>
> Thanks,
>
> Maxim Khutornenko
>
>
Re: Review Request 24702: Implementing client job lock and start
update APIs.
Posted by Bill Farner <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24702/#review50598
-----------------------------------------------------------
LGTM, would like to reconcile the populateJobConfig situation before proceeding.
src/main/python/apache/aurora/client/api/__init__.py
<https://reviews.apache.org/r/24702/#comment88467>
previously-acquired
src/main/python/apache/aurora/client/api/__init__.py
<https://reviews.apache.org/r/24702/#comment88468>
Seems like the scheduler should automatically do this within startJobUpdate. Is there anything preventing that behavior?
- Bill Farner
On Aug. 14, 2014, 4:08 p.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24702/
> -----------------------------------------------------------
>
> (Updated Aug. 14, 2014, 4:08 p.m.)
>
>
> Review request for Aurora, Mark Chu-Carroll and Bill Farner.
>
>
> Bugs: AURORA-615
> https://issues.apache.org/jira/browse/AURORA-615
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Implementing client job lock and start update APIs.
>
>
> Diffs
> -----
>
> src/main/python/apache/aurora/client/api/__init__.py 371137e469fd65935f4048eaafdcf9b702e8c947
> src/main/python/apache/aurora/client/api/updater_util.py cbb93e4d64a334dba9d091b68cf6d3594930fa31
> src/test/python/apache/aurora/client/api/BUILD db5c22375378566f58ade5d6a0b7fcce75ad8394
> src/test/python/apache/aurora/client/api/test_api.py PRE-CREATION
>
> Diff: https://reviews.apache.org/r/24702/diff/
>
>
> Testing
> -------
>
> ./pants src/test/python/apache/aurora/client/api:api
>
>
> Thanks,
>
> Maxim Khutornenko
>
>
Re: Review Request 24702: Implementing client job lock and start
update APIs.
Posted by Mark Chu-Carroll <mc...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24702/#review50619
-----------------------------------------------------------
Ship it!
Ship It!
- Mark Chu-Carroll
On Aug. 14, 2014, 2:26 p.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24702/
> -----------------------------------------------------------
>
> (Updated Aug. 14, 2014, 2:26 p.m.)
>
>
> Review request for Aurora, Mark Chu-Carroll and Bill Farner.
>
>
> Bugs: AURORA-615
> https://issues.apache.org/jira/browse/AURORA-615
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Implementing client job lock and start update APIs.
>
>
> Diffs
> -----
>
> src/main/python/apache/aurora/client/api/__init__.py 371137e469fd65935f4048eaafdcf9b702e8c947
> src/main/python/apache/aurora/client/api/updater_util.py cbb93e4d64a334dba9d091b68cf6d3594930fa31
> src/test/python/apache/aurora/client/api/BUILD db5c22375378566f58ade5d6a0b7fcce75ad8394
> src/test/python/apache/aurora/client/api/test_api.py PRE-CREATION
> src/test/python/apache/aurora/client/api/test_updater_util.py PRE-CREATION
>
> Diff: https://reviews.apache.org/r/24702/diff/
>
>
> Testing
> -------
>
> ./pants src/test/python/apache/aurora/client/api:api
>
>
> Thanks,
>
> Maxim Khutornenko
>
>
Re: Review Request 24702: Implementing client job lock and start
update APIs.
Posted by Bill Farner <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24702/#review50674
-----------------------------------------------------------
Ship it!
Ship It!
- Bill Farner
On Aug. 14, 2014, 6:26 p.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24702/
> -----------------------------------------------------------
>
> (Updated Aug. 14, 2014, 6:26 p.m.)
>
>
> Review request for Aurora, Mark Chu-Carroll and Bill Farner.
>
>
> Bugs: AURORA-615
> https://issues.apache.org/jira/browse/AURORA-615
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Implementing client job lock and start update APIs.
>
>
> Diffs
> -----
>
> src/main/python/apache/aurora/client/api/__init__.py 371137e469fd65935f4048eaafdcf9b702e8c947
> src/main/python/apache/aurora/client/api/updater_util.py cbb93e4d64a334dba9d091b68cf6d3594930fa31
> src/test/python/apache/aurora/client/api/BUILD db5c22375378566f58ade5d6a0b7fcce75ad8394
> src/test/python/apache/aurora/client/api/test_api.py PRE-CREATION
> src/test/python/apache/aurora/client/api/test_updater_util.py PRE-CREATION
>
> Diff: https://reviews.apache.org/r/24702/diff/
>
>
> Testing
> -------
>
> ./pants src/test/python/apache/aurora/client/api:api
>
>
> Thanks,
>
> Maxim Khutornenko
>
>
Re: Review Request 24702: Implementing client job lock and start
update APIs.
Posted by Maxim Khutornenko <ma...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24702/
-----------------------------------------------------------
(Updated Aug. 14, 2014, 6:26 p.m.)
Review request for Aurora, Mark Chu-Carroll and Bill Farner.
Changes
-------
CR comments.
Bugs: AURORA-615
https://issues.apache.org/jira/browse/AURORA-615
Repository: aurora
Description
-------
Implementing client job lock and start update APIs.
Diffs (updated)
-----
src/main/python/apache/aurora/client/api/__init__.py 371137e469fd65935f4048eaafdcf9b702e8c947
src/main/python/apache/aurora/client/api/updater_util.py cbb93e4d64a334dba9d091b68cf6d3594930fa31
src/test/python/apache/aurora/client/api/BUILD db5c22375378566f58ade5d6a0b7fcce75ad8394
src/test/python/apache/aurora/client/api/test_api.py PRE-CREATION
src/test/python/apache/aurora/client/api/test_updater_util.py PRE-CREATION
Diff: https://reviews.apache.org/r/24702/diff/
Testing
-------
./pants src/test/python/apache/aurora/client/api:api
Thanks,
Maxim Khutornenko
Re: Review Request 24702: Implementing client job lock and start
update APIs.
Posted by Maxim Khutornenko <ma...@apache.org>.
> On Aug. 14, 2014, 5:09 p.m., Mark Chu-Carroll wrote:
> > src/main/python/apache/aurora/client/api/updater_util.py, line 67
> > <https://reviews.apache.org/r/24702/diff/1/?file=660625#file660625line67>
> >
> > This is a bit magical for me. It's relying on subtleties of methods that a typical python programmer will need to look up, which means that it's going to be really easy for it to be wrong in a hard-to-recognize way.
> >
> > Can you move it up, so that it's a classmethod, and then add a couple of tests? Tests will both check that it works, and provide demonstrations for readers to see how it works. (I know you've already got some tests of this, but they're indirect, htrough the surrounding method; for clarity, it would be better to be able to test get_ranges directly.)
Moved into a classmethod, added comments and moved unit tests into a separate file.
> On Aug. 14, 2014, 5:09 p.m., Mark Chu-Carroll wrote:
> > src/main/python/apache/aurora/client/api/__init__.py, line 179
> > <https://reviews.apache.org/r/24702/diff/1/?file=660624#file660624line179>
> >
> > Can you add a more specific error type - maybe InvalidConfigError? That way, in the client, I can generate better error messages.
Sure, done.
- Maxim
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24702/#review50593
-----------------------------------------------------------
On Aug. 14, 2014, 4:08 p.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24702/
> -----------------------------------------------------------
>
> (Updated Aug. 14, 2014, 4:08 p.m.)
>
>
> Review request for Aurora, Mark Chu-Carroll and Bill Farner.
>
>
> Bugs: AURORA-615
> https://issues.apache.org/jira/browse/AURORA-615
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Implementing client job lock and start update APIs.
>
>
> Diffs
> -----
>
> src/main/python/apache/aurora/client/api/__init__.py 371137e469fd65935f4048eaafdcf9b702e8c947
> src/main/python/apache/aurora/client/api/updater_util.py cbb93e4d64a334dba9d091b68cf6d3594930fa31
> src/test/python/apache/aurora/client/api/BUILD db5c22375378566f58ade5d6a0b7fcce75ad8394
> src/test/python/apache/aurora/client/api/test_api.py PRE-CREATION
>
> Diff: https://reviews.apache.org/r/24702/diff/
>
>
> Testing
> -------
>
> ./pants src/test/python/apache/aurora/client/api:api
>
>
> Thanks,
>
> Maxim Khutornenko
>
>
Re: Review Request 24702: Implementing client job lock and start
update APIs.
Posted by Mark Chu-Carroll <mc...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24702/#review50593
-----------------------------------------------------------
src/main/python/apache/aurora/client/api/__init__.py
<https://reviews.apache.org/r/24702/#comment88457>
Can you add a more specific error type - maybe InvalidConfigError? That way, in the client, I can generate better error messages.
src/main/python/apache/aurora/client/api/updater_util.py
<https://reviews.apache.org/r/24702/#comment88462>
This is a bit magical for me. It's relying on subtleties of methods that a typical python programmer will need to look up, which means that it's going to be really easy for it to be wrong in a hard-to-recognize way.
Can you move it up, so that it's a classmethod, and then add a couple of tests? Tests will both check that it works, and provide demonstrations for readers to see how it works. (I know you've already got some tests of this, but they're indirect, htrough the surrounding method; for clarity, it would be better to be able to test get_ranges directly.)
- Mark Chu-Carroll
On Aug. 14, 2014, 12:08 p.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24702/
> -----------------------------------------------------------
>
> (Updated Aug. 14, 2014, 12:08 p.m.)
>
>
> Review request for Aurora, Mark Chu-Carroll and Bill Farner.
>
>
> Bugs: AURORA-615
> https://issues.apache.org/jira/browse/AURORA-615
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Implementing client job lock and start update APIs.
>
>
> Diffs
> -----
>
> src/main/python/apache/aurora/client/api/__init__.py 371137e469fd65935f4048eaafdcf9b702e8c947
> src/main/python/apache/aurora/client/api/updater_util.py cbb93e4d64a334dba9d091b68cf6d3594930fa31
> src/test/python/apache/aurora/client/api/BUILD db5c22375378566f58ade5d6a0b7fcce75ad8394
> src/test/python/apache/aurora/client/api/test_api.py PRE-CREATION
>
> Diff: https://reviews.apache.org/r/24702/diff/
>
>
> Testing
> -------
>
> ./pants src/test/python/apache/aurora/client/api:api
>
>
> Thanks,
>
> Maxim Khutornenko
>
>