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 2016/02/09 20:44:24 UTC
Review Request 43373: Implementing 'aurora job add' command.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43373/
-----------------------------------------------------------
Review request for Aurora, John Sirois and Bill Farner.
Bugs: AURORA-1258
https://issues.apache.org/jira/browse/AURORA-1258
Repository: aurora
Description
-------
Taking advantage of the refactored addInstances RPC to add a 'scale out' client command.
The mitigation of .aurora config file `instances` value mismatch is currently discussed at http://markmail.org/message/5ekpapmxr4vxxcd5 and will be addressed separately.
Diffs
-----
src/main/python/apache/aurora/client/api/__init__.py 63bd649377816c72ef2453d9abb7363637a91680
src/main/python/apache/aurora/client/cli/context.py 24a37ec60a8845b4816d292c49fed814ba0a5854
src/main/python/apache/aurora/client/cli/jobs.py deba3a9026384e5bd436e611ab5b5ae3afc1cfcc
src/main/python/apache/aurora/client/cli/options.py 226397865e54cf4c0a59ecb91b93ddc10967edf7
src/main/python/apache/aurora/client/hooks/hooked_api.py 185e57d08aa13b687529fefd7c356f3d50c6af33
src/test/python/apache/aurora/api_util.py 9d44b884e5e67506acaaecda1d9ff02a80cf1567
src/test/python/apache/aurora/client/api/test_api.py 1f041f42fcd5ee5d147afacd9c8f83aa3129fe47
src/test/python/apache/aurora/client/cli/test_add.py PRE-CREATION
src/test/python/apache/aurora/client/hooks/test_hooked_api.py 67517a23535e3f535c3114193a5ae06bf4326515
src/test/python/apache/aurora/client/hooks/test_non_hooked_api.py f4b771bc968143845772a0149a943c97c5aaf963
Diff: https://reviews.apache.org/r/43373/diff/
Testing
-------
local and manual tests in vagrant
Thanks,
Maxim Khutornenko
Re: Review Request 43373: Implementing 'aurora job add' command.
Posted by Aurora ReviewBot <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43373/#review118485
-----------------------------------------------------------
Ship it!
Master (dd79934) is green with this patch.
./build-support/jenkins/build.sh
I will refresh this build result if you post a review containing "@ReviewBot retry"
- Aurora ReviewBot
On Feb. 9, 2016, 10:04 p.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43373/
> -----------------------------------------------------------
>
> (Updated Feb. 9, 2016, 10:04 p.m.)
>
>
> Review request for Aurora, John Sirois and Bill Farner.
>
>
> Bugs: AURORA-1258
> https://issues.apache.org/jira/browse/AURORA-1258
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Taking advantage of the refactored addInstances RPC to add a 'scale out' client command.
>
> The mitigation of .aurora config file `instances` value mismatch is currently discussed at http://markmail.org/message/5ekpapmxr4vxxcd5 and will be addressed separately.
>
>
> Diffs
> -----
>
> src/main/python/apache/aurora/client/api/__init__.py 63bd649377816c72ef2453d9abb7363637a91680
> src/main/python/apache/aurora/client/cli/context.py 24a37ec60a8845b4816d292c49fed814ba0a5854
> src/main/python/apache/aurora/client/cli/jobs.py deba3a9026384e5bd436e611ab5b5ae3afc1cfcc
> src/main/python/apache/aurora/client/cli/options.py 226397865e54cf4c0a59ecb91b93ddc10967edf7
> src/main/python/apache/aurora/client/hooks/hooked_api.py 185e57d08aa13b687529fefd7c356f3d50c6af33
> src/test/python/apache/aurora/api_util.py 9d44b884e5e67506acaaecda1d9ff02a80cf1567
> src/test/python/apache/aurora/client/api/test_api.py 1f041f42fcd5ee5d147afacd9c8f83aa3129fe47
> src/test/python/apache/aurora/client/cli/test_add.py PRE-CREATION
> src/test/python/apache/aurora/client/cli/test_options.py 21d5888c0a87e07a385159de0b56420086666d9d
> src/test/python/apache/aurora/client/hooks/test_hooked_api.py 67517a23535e3f535c3114193a5ae06bf4326515
> src/test/python/apache/aurora/client/hooks/test_non_hooked_api.py f4b771bc968143845772a0149a943c97c5aaf963
>
> Diff: https://reviews.apache.org/r/43373/diff/
>
>
> Testing
> -------
>
> local and manual tests in vagrant
>
>
> Thanks,
>
> Maxim Khutornenko
>
>
Re: Review Request 43373: Implementing 'aurora job add' command.
Posted by Aurora ReviewBot <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43373/#review118706
-----------------------------------------------------------
Ship it!
Master (86b6d79) is green with this patch.
./build-support/jenkins/build.sh
I will refresh this build result if you post a review containing "@ReviewBot retry"
- Aurora ReviewBot
On Feb. 10, 2016, 6:33 p.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43373/
> -----------------------------------------------------------
>
> (Updated Feb. 10, 2016, 6:33 p.m.)
>
>
> Review request for Aurora, John Sirois and Bill Farner.
>
>
> Bugs: AURORA-1258
> https://issues.apache.org/jira/browse/AURORA-1258
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Taking advantage of the refactored addInstances RPC to add a 'scale out' client command.
>
> The mitigation of .aurora config file `instances` value mismatch is currently discussed at http://markmail.org/message/5ekpapmxr4vxxcd5 and will be addressed separately.
>
>
> Diffs
> -----
>
> src/main/python/apache/aurora/client/api/__init__.py 63bd649377816c72ef2453d9abb7363637a91680
> src/main/python/apache/aurora/client/cli/context.py 24a37ec60a8845b4816d292c49fed814ba0a5854
> src/main/python/apache/aurora/client/cli/jobs.py 67ab4f01eca911aea041328410c6104b63ff8344
> src/main/python/apache/aurora/client/cli/options.py 226397865e54cf4c0a59ecb91b93ddc10967edf7
> src/main/python/apache/aurora/client/hooks/hooked_api.py 185e57d08aa13b687529fefd7c356f3d50c6af33
> src/test/python/apache/aurora/api_util.py 9d44b884e5e67506acaaecda1d9ff02a80cf1567
> src/test/python/apache/aurora/client/api/test_api.py 974fc7e5a8ea13020e2c01a2088a8535947f05bc
> src/test/python/apache/aurora/client/cli/test_add.py PRE-CREATION
> src/test/python/apache/aurora/client/cli/test_options.py 21d5888c0a87e07a385159de0b56420086666d9d
> src/test/python/apache/aurora/client/hooks/test_hooked_api.py 67517a23535e3f535c3114193a5ae06bf4326515
> src/test/python/apache/aurora/client/hooks/test_non_hooked_api.py f4b771bc968143845772a0149a943c97c5aaf963
>
> Diff: https://reviews.apache.org/r/43373/diff/
>
>
> Testing
> -------
>
> local and manual tests in vagrant
>
>
> Thanks,
>
> Maxim Khutornenko
>
>
Re: Review Request 43373: Implementing 'aurora job add' command.
Posted by John Sirois <js...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43373/#review118699
-----------------------------------------------------------
Ship it!
Ship It!
- John Sirois
On Feb. 10, 2016, 11:33 a.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43373/
> -----------------------------------------------------------
>
> (Updated Feb. 10, 2016, 11:33 a.m.)
>
>
> Review request for Aurora, John Sirois and Bill Farner.
>
>
> Bugs: AURORA-1258
> https://issues.apache.org/jira/browse/AURORA-1258
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Taking advantage of the refactored addInstances RPC to add a 'scale out' client command.
>
> The mitigation of .aurora config file `instances` value mismatch is currently discussed at http://markmail.org/message/5ekpapmxr4vxxcd5 and will be addressed separately.
>
>
> Diffs
> -----
>
> src/main/python/apache/aurora/client/api/__init__.py 63bd649377816c72ef2453d9abb7363637a91680
> src/main/python/apache/aurora/client/cli/context.py 24a37ec60a8845b4816d292c49fed814ba0a5854
> src/main/python/apache/aurora/client/cli/jobs.py 67ab4f01eca911aea041328410c6104b63ff8344
> src/main/python/apache/aurora/client/cli/options.py 226397865e54cf4c0a59ecb91b93ddc10967edf7
> src/main/python/apache/aurora/client/hooks/hooked_api.py 185e57d08aa13b687529fefd7c356f3d50c6af33
> src/test/python/apache/aurora/api_util.py 9d44b884e5e67506acaaecda1d9ff02a80cf1567
> src/test/python/apache/aurora/client/api/test_api.py 974fc7e5a8ea13020e2c01a2088a8535947f05bc
> src/test/python/apache/aurora/client/cli/test_add.py PRE-CREATION
> src/test/python/apache/aurora/client/cli/test_options.py 21d5888c0a87e07a385159de0b56420086666d9d
> src/test/python/apache/aurora/client/hooks/test_hooked_api.py 67517a23535e3f535c3114193a5ae06bf4326515
> src/test/python/apache/aurora/client/hooks/test_non_hooked_api.py f4b771bc968143845772a0149a943c97c5aaf963
>
> Diff: https://reviews.apache.org/r/43373/diff/
>
>
> Testing
> -------
>
> local and manual tests in vagrant
>
>
> Thanks,
>
> Maxim Khutornenko
>
>
Re: Review Request 43373: Implementing 'aurora job add' command.
Posted by Joshua Cohen <jc...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43373/#review118707
-----------------------------------------------------------
Ship it!
Ship It!
- Joshua Cohen
On Feb. 10, 2016, 6:33 p.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43373/
> -----------------------------------------------------------
>
> (Updated Feb. 10, 2016, 6:33 p.m.)
>
>
> Review request for Aurora, John Sirois and Bill Farner.
>
>
> Bugs: AURORA-1258
> https://issues.apache.org/jira/browse/AURORA-1258
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Taking advantage of the refactored addInstances RPC to add a 'scale out' client command.
>
> The mitigation of .aurora config file `instances` value mismatch is currently discussed at http://markmail.org/message/5ekpapmxr4vxxcd5 and will be addressed separately.
>
>
> Diffs
> -----
>
> src/main/python/apache/aurora/client/api/__init__.py 63bd649377816c72ef2453d9abb7363637a91680
> src/main/python/apache/aurora/client/cli/context.py 24a37ec60a8845b4816d292c49fed814ba0a5854
> src/main/python/apache/aurora/client/cli/jobs.py 67ab4f01eca911aea041328410c6104b63ff8344
> src/main/python/apache/aurora/client/cli/options.py 226397865e54cf4c0a59ecb91b93ddc10967edf7
> src/main/python/apache/aurora/client/hooks/hooked_api.py 185e57d08aa13b687529fefd7c356f3d50c6af33
> src/test/python/apache/aurora/api_util.py 9d44b884e5e67506acaaecda1d9ff02a80cf1567
> src/test/python/apache/aurora/client/api/test_api.py 974fc7e5a8ea13020e2c01a2088a8535947f05bc
> src/test/python/apache/aurora/client/cli/test_add.py PRE-CREATION
> src/test/python/apache/aurora/client/cli/test_options.py 21d5888c0a87e07a385159de0b56420086666d9d
> src/test/python/apache/aurora/client/hooks/test_hooked_api.py 67517a23535e3f535c3114193a5ae06bf4326515
> src/test/python/apache/aurora/client/hooks/test_non_hooked_api.py f4b771bc968143845772a0149a943c97c5aaf963
>
> Diff: https://reviews.apache.org/r/43373/diff/
>
>
> Testing
> -------
>
> local and manual tests in vagrant
>
>
> Thanks,
>
> Maxim Khutornenko
>
>
Re: Review Request 43373: Implementing 'aurora job add' command.
Posted by Bill Farner <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43373/#review118703
-----------------------------------------------------------
Sorry to back out late - i don't have capacity for a decent review and need to step out. Feel free to swap me out or proceed with just you and John.
- Bill Farner
On Feb. 10, 2016, 10:33 a.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43373/
> -----------------------------------------------------------
>
> (Updated Feb. 10, 2016, 10:33 a.m.)
>
>
> Review request for Aurora, John Sirois and Bill Farner.
>
>
> Bugs: AURORA-1258
> https://issues.apache.org/jira/browse/AURORA-1258
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Taking advantage of the refactored addInstances RPC to add a 'scale out' client command.
>
> The mitigation of .aurora config file `instances` value mismatch is currently discussed at http://markmail.org/message/5ekpapmxr4vxxcd5 and will be addressed separately.
>
>
> Diffs
> -----
>
> src/main/python/apache/aurora/client/api/__init__.py 63bd649377816c72ef2453d9abb7363637a91680
> src/main/python/apache/aurora/client/cli/context.py 24a37ec60a8845b4816d292c49fed814ba0a5854
> src/main/python/apache/aurora/client/cli/jobs.py 67ab4f01eca911aea041328410c6104b63ff8344
> src/main/python/apache/aurora/client/cli/options.py 226397865e54cf4c0a59ecb91b93ddc10967edf7
> src/main/python/apache/aurora/client/hooks/hooked_api.py 185e57d08aa13b687529fefd7c356f3d50c6af33
> src/test/python/apache/aurora/api_util.py 9d44b884e5e67506acaaecda1d9ff02a80cf1567
> src/test/python/apache/aurora/client/api/test_api.py 974fc7e5a8ea13020e2c01a2088a8535947f05bc
> src/test/python/apache/aurora/client/cli/test_add.py PRE-CREATION
> src/test/python/apache/aurora/client/cli/test_options.py 21d5888c0a87e07a385159de0b56420086666d9d
> src/test/python/apache/aurora/client/hooks/test_hooked_api.py 67517a23535e3f535c3114193a5ae06bf4326515
> src/test/python/apache/aurora/client/hooks/test_non_hooked_api.py f4b771bc968143845772a0149a943c97c5aaf963
>
> Diff: https://reviews.apache.org/r/43373/diff/
>
>
> Testing
> -------
>
> local and manual tests in vagrant
>
>
> Thanks,
>
> Maxim Khutornenko
>
>
Re: Review Request 43373: Implementing 'aurora job add' command.
Posted by Stephan Erb <se...@apache.org>.
> On Feb. 10, 2016, 10:39 p.m., Stephan Erb wrote:
> > Stupid question: Will there be another review request implementing instance removal?
>
> Maxim Khutornenko wrote:
> It already exists: `aurora job kill`
>
> Stephan Erb wrote:
> I feel like this exposes an inconsistency of the API:
>
> * `add` appends some instances, without me ever telling me about which instance IDs have been added.
> * `kill` forces me to explicitly list the instance IDs I want to remove.
>
> This makes it difficult to undo the addition of instances.
>
> Maxim Khutornenko wrote:
> Sorry, just noticed your comment after pushing it to master. I think having this type of assymetry should be expected as killing instances does not bear the same SLA meaning as adding them. In other words, I think killing instances should be a more involved operation than adding them to prevent catastrophic service outages due to misspelled instance count. We already have a `job kill` command that is well vetted against that type of action:
> 1. It does not let you kill the entire instance set
> 2. It requires explicit instance ranges
> 3. It uses batch_size=1 (default)
I see. Tool safety is a valid point.
- Stephan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43373/#review118728
-----------------------------------------------------------
On Feb. 11, 2016, 12:11 a.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43373/
> -----------------------------------------------------------
>
> (Updated Feb. 11, 2016, 12:11 a.m.)
>
>
> Review request for Aurora, Joshua Cohen and John Sirois.
>
>
> Bugs: AURORA-1258
> https://issues.apache.org/jira/browse/AURORA-1258
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Taking advantage of the refactored addInstances RPC to add a 'scale out' client command.
>
> The mitigation of .aurora config file `instances` value mismatch is currently discussed at http://markmail.org/message/5ekpapmxr4vxxcd5 and will be addressed separately.
>
>
> Diffs
> -----
>
> src/main/python/apache/aurora/client/api/__init__.py 63bd649377816c72ef2453d9abb7363637a91680
> src/main/python/apache/aurora/client/cli/context.py 24a37ec60a8845b4816d292c49fed814ba0a5854
> src/main/python/apache/aurora/client/cli/jobs.py 67ab4f01eca911aea041328410c6104b63ff8344
> src/main/python/apache/aurora/client/cli/options.py 226397865e54cf4c0a59ecb91b93ddc10967edf7
> src/main/python/apache/aurora/client/hooks/hooked_api.py 185e57d08aa13b687529fefd7c356f3d50c6af33
> src/test/python/apache/aurora/api_util.py 9d44b884e5e67506acaaecda1d9ff02a80cf1567
> src/test/python/apache/aurora/client/api/test_api.py 974fc7e5a8ea13020e2c01a2088a8535947f05bc
> src/test/python/apache/aurora/client/cli/test_add.py PRE-CREATION
> src/test/python/apache/aurora/client/cli/test_options.py 21d5888c0a87e07a385159de0b56420086666d9d
> src/test/python/apache/aurora/client/hooks/test_hooked_api.py 67517a23535e3f535c3114193a5ae06bf4326515
> src/test/python/apache/aurora/client/hooks/test_non_hooked_api.py f4b771bc968143845772a0149a943c97c5aaf963
>
> Diff: https://reviews.apache.org/r/43373/diff/
>
>
> Testing
> -------
>
> local and manual tests in vagrant
>
>
> Thanks,
>
> Maxim Khutornenko
>
>
Re: Review Request 43373: Implementing 'aurora job add' command.
Posted by Maxim Khutornenko <ma...@apache.org>.
> On Feb. 10, 2016, 9:39 p.m., Stephan Erb wrote:
> > Stupid question: Will there be another review request implementing instance removal?
It already exists: `aurora job kill`
- Maxim
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43373/#review118728
-----------------------------------------------------------
On Feb. 10, 2016, 6:33 p.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43373/
> -----------------------------------------------------------
>
> (Updated Feb. 10, 2016, 6:33 p.m.)
>
>
> Review request for Aurora, John Sirois and Bill Farner.
>
>
> Bugs: AURORA-1258
> https://issues.apache.org/jira/browse/AURORA-1258
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Taking advantage of the refactored addInstances RPC to add a 'scale out' client command.
>
> The mitigation of .aurora config file `instances` value mismatch is currently discussed at http://markmail.org/message/5ekpapmxr4vxxcd5 and will be addressed separately.
>
>
> Diffs
> -----
>
> src/main/python/apache/aurora/client/api/__init__.py 63bd649377816c72ef2453d9abb7363637a91680
> src/main/python/apache/aurora/client/cli/context.py 24a37ec60a8845b4816d292c49fed814ba0a5854
> src/main/python/apache/aurora/client/cli/jobs.py 67ab4f01eca911aea041328410c6104b63ff8344
> src/main/python/apache/aurora/client/cli/options.py 226397865e54cf4c0a59ecb91b93ddc10967edf7
> src/main/python/apache/aurora/client/hooks/hooked_api.py 185e57d08aa13b687529fefd7c356f3d50c6af33
> src/test/python/apache/aurora/api_util.py 9d44b884e5e67506acaaecda1d9ff02a80cf1567
> src/test/python/apache/aurora/client/api/test_api.py 974fc7e5a8ea13020e2c01a2088a8535947f05bc
> src/test/python/apache/aurora/client/cli/test_add.py PRE-CREATION
> src/test/python/apache/aurora/client/cli/test_options.py 21d5888c0a87e07a385159de0b56420086666d9d
> src/test/python/apache/aurora/client/hooks/test_hooked_api.py 67517a23535e3f535c3114193a5ae06bf4326515
> src/test/python/apache/aurora/client/hooks/test_non_hooked_api.py f4b771bc968143845772a0149a943c97c5aaf963
>
> Diff: https://reviews.apache.org/r/43373/diff/
>
>
> Testing
> -------
>
> local and manual tests in vagrant
>
>
> Thanks,
>
> Maxim Khutornenko
>
>
Re: Review Request 43373: Implementing 'aurora job add' command.
Posted by Maxim Khutornenko <ma...@apache.org>.
> On Feb. 10, 2016, 9:39 p.m., Stephan Erb wrote:
> > Stupid question: Will there be another review request implementing instance removal?
>
> Maxim Khutornenko wrote:
> It already exists: `aurora job kill`
>
> Stephan Erb wrote:
> I feel like this exposes an inconsistency of the API:
>
> * `add` appends some instances, without me ever telling me about which instance IDs have been added.
> * `kill` forces me to explicitly list the instance IDs I want to remove.
>
> This makes it difficult to undo the addition of instances.
Sorry, just noticed your comment after pushing it to master. I think having this type of assymetry should be expected as killing instances does not bear the same SLA meaning as adding them. In other words, I think killing instances should be a more involved operation than adding them to prevent catastrophic service outages due to misspelled instance count. We already have a `job kill` command that is well vetted against that type of action:
1. It does not let you kill the entire instance set
2. It requires explicit instance ranges
3. It uses batch_size=1 (default)
- Maxim
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43373/#review118728
-----------------------------------------------------------
On Feb. 10, 2016, 11:11 p.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43373/
> -----------------------------------------------------------
>
> (Updated Feb. 10, 2016, 11:11 p.m.)
>
>
> Review request for Aurora, Joshua Cohen and John Sirois.
>
>
> Bugs: AURORA-1258
> https://issues.apache.org/jira/browse/AURORA-1258
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Taking advantage of the refactored addInstances RPC to add a 'scale out' client command.
>
> The mitigation of .aurora config file `instances` value mismatch is currently discussed at http://markmail.org/message/5ekpapmxr4vxxcd5 and will be addressed separately.
>
>
> Diffs
> -----
>
> src/main/python/apache/aurora/client/api/__init__.py 63bd649377816c72ef2453d9abb7363637a91680
> src/main/python/apache/aurora/client/cli/context.py 24a37ec60a8845b4816d292c49fed814ba0a5854
> src/main/python/apache/aurora/client/cli/jobs.py 67ab4f01eca911aea041328410c6104b63ff8344
> src/main/python/apache/aurora/client/cli/options.py 226397865e54cf4c0a59ecb91b93ddc10967edf7
> src/main/python/apache/aurora/client/hooks/hooked_api.py 185e57d08aa13b687529fefd7c356f3d50c6af33
> src/test/python/apache/aurora/api_util.py 9d44b884e5e67506acaaecda1d9ff02a80cf1567
> src/test/python/apache/aurora/client/api/test_api.py 974fc7e5a8ea13020e2c01a2088a8535947f05bc
> src/test/python/apache/aurora/client/cli/test_add.py PRE-CREATION
> src/test/python/apache/aurora/client/cli/test_options.py 21d5888c0a87e07a385159de0b56420086666d9d
> src/test/python/apache/aurora/client/hooks/test_hooked_api.py 67517a23535e3f535c3114193a5ae06bf4326515
> src/test/python/apache/aurora/client/hooks/test_non_hooked_api.py f4b771bc968143845772a0149a943c97c5aaf963
>
> Diff: https://reviews.apache.org/r/43373/diff/
>
>
> Testing
> -------
>
> local and manual tests in vagrant
>
>
> Thanks,
>
> Maxim Khutornenko
>
>
Re: Review Request 43373: Implementing 'aurora job add' command.
Posted by Stephan Erb <se...@apache.org>.
> On Feb. 10, 2016, 10:39 p.m., Stephan Erb wrote:
> > Stupid question: Will there be another review request implementing instance removal?
>
> Maxim Khutornenko wrote:
> It already exists: `aurora job kill`
I feel like this exposes an inconsistency of the API:
* `add` appends some instances, without me ever telling me about which instance IDs have been added.
* `kill` forces me to explicitly list the instance IDs I want to remove.
This makes it difficult to undo the addition of instances.
- Stephan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43373/#review118728
-----------------------------------------------------------
On Feb. 10, 2016, 7:33 p.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43373/
> -----------------------------------------------------------
>
> (Updated Feb. 10, 2016, 7:33 p.m.)
>
>
> Review request for Aurora, John Sirois and Bill Farner.
>
>
> Bugs: AURORA-1258
> https://issues.apache.org/jira/browse/AURORA-1258
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Taking advantage of the refactored addInstances RPC to add a 'scale out' client command.
>
> The mitigation of .aurora config file `instances` value mismatch is currently discussed at http://markmail.org/message/5ekpapmxr4vxxcd5 and will be addressed separately.
>
>
> Diffs
> -----
>
> src/main/python/apache/aurora/client/api/__init__.py 63bd649377816c72ef2453d9abb7363637a91680
> src/main/python/apache/aurora/client/cli/context.py 24a37ec60a8845b4816d292c49fed814ba0a5854
> src/main/python/apache/aurora/client/cli/jobs.py 67ab4f01eca911aea041328410c6104b63ff8344
> src/main/python/apache/aurora/client/cli/options.py 226397865e54cf4c0a59ecb91b93ddc10967edf7
> src/main/python/apache/aurora/client/hooks/hooked_api.py 185e57d08aa13b687529fefd7c356f3d50c6af33
> src/test/python/apache/aurora/api_util.py 9d44b884e5e67506acaaecda1d9ff02a80cf1567
> src/test/python/apache/aurora/client/api/test_api.py 974fc7e5a8ea13020e2c01a2088a8535947f05bc
> src/test/python/apache/aurora/client/cli/test_add.py PRE-CREATION
> src/test/python/apache/aurora/client/cli/test_options.py 21d5888c0a87e07a385159de0b56420086666d9d
> src/test/python/apache/aurora/client/hooks/test_hooked_api.py 67517a23535e3f535c3114193a5ae06bf4326515
> src/test/python/apache/aurora/client/hooks/test_non_hooked_api.py f4b771bc968143845772a0149a943c97c5aaf963
>
> Diff: https://reviews.apache.org/r/43373/diff/
>
>
> Testing
> -------
>
> local and manual tests in vagrant
>
>
> Thanks,
>
> Maxim Khutornenko
>
>
Re: Review Request 43373: Implementing 'aurora job add' command.
Posted by Stephan Erb <se...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43373/#review118728
-----------------------------------------------------------
Stupid question: Will there be another review request implementing instance removal?
- Stephan Erb
On Feb. 10, 2016, 7:33 p.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43373/
> -----------------------------------------------------------
>
> (Updated Feb. 10, 2016, 7:33 p.m.)
>
>
> Review request for Aurora, John Sirois and Bill Farner.
>
>
> Bugs: AURORA-1258
> https://issues.apache.org/jira/browse/AURORA-1258
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Taking advantage of the refactored addInstances RPC to add a 'scale out' client command.
>
> The mitigation of .aurora config file `instances` value mismatch is currently discussed at http://markmail.org/message/5ekpapmxr4vxxcd5 and will be addressed separately.
>
>
> Diffs
> -----
>
> src/main/python/apache/aurora/client/api/__init__.py 63bd649377816c72ef2453d9abb7363637a91680
> src/main/python/apache/aurora/client/cli/context.py 24a37ec60a8845b4816d292c49fed814ba0a5854
> src/main/python/apache/aurora/client/cli/jobs.py 67ab4f01eca911aea041328410c6104b63ff8344
> src/main/python/apache/aurora/client/cli/options.py 226397865e54cf4c0a59ecb91b93ddc10967edf7
> src/main/python/apache/aurora/client/hooks/hooked_api.py 185e57d08aa13b687529fefd7c356f3d50c6af33
> src/test/python/apache/aurora/api_util.py 9d44b884e5e67506acaaecda1d9ff02a80cf1567
> src/test/python/apache/aurora/client/api/test_api.py 974fc7e5a8ea13020e2c01a2088a8535947f05bc
> src/test/python/apache/aurora/client/cli/test_add.py PRE-CREATION
> src/test/python/apache/aurora/client/cli/test_options.py 21d5888c0a87e07a385159de0b56420086666d9d
> src/test/python/apache/aurora/client/hooks/test_hooked_api.py 67517a23535e3f535c3114193a5ae06bf4326515
> src/test/python/apache/aurora/client/hooks/test_non_hooked_api.py f4b771bc968143845772a0149a943c97c5aaf963
>
> Diff: https://reviews.apache.org/r/43373/diff/
>
>
> Testing
> -------
>
> local and manual tests in vagrant
>
>
> Thanks,
>
> Maxim Khutornenko
>
>
Re: Review Request 43373: Implementing 'aurora job add' command.
Posted by Maxim Khutornenko <ma...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43373/
-----------------------------------------------------------
(Updated Feb. 10, 2016, 6:33 p.m.)
Review request for Aurora, John Sirois and Bill Farner.
Changes
-------
John's comments.
Bugs: AURORA-1258
https://issues.apache.org/jira/browse/AURORA-1258
Repository: aurora
Description
-------
Taking advantage of the refactored addInstances RPC to add a 'scale out' client command.
The mitigation of .aurora config file `instances` value mismatch is currently discussed at http://markmail.org/message/5ekpapmxr4vxxcd5 and will be addressed separately.
Diffs (updated)
-----
src/main/python/apache/aurora/client/api/__init__.py 63bd649377816c72ef2453d9abb7363637a91680
src/main/python/apache/aurora/client/cli/context.py 24a37ec60a8845b4816d292c49fed814ba0a5854
src/main/python/apache/aurora/client/cli/jobs.py 67ab4f01eca911aea041328410c6104b63ff8344
src/main/python/apache/aurora/client/cli/options.py 226397865e54cf4c0a59ecb91b93ddc10967edf7
src/main/python/apache/aurora/client/hooks/hooked_api.py 185e57d08aa13b687529fefd7c356f3d50c6af33
src/test/python/apache/aurora/api_util.py 9d44b884e5e67506acaaecda1d9ff02a80cf1567
src/test/python/apache/aurora/client/api/test_api.py 974fc7e5a8ea13020e2c01a2088a8535947f05bc
src/test/python/apache/aurora/client/cli/test_add.py PRE-CREATION
src/test/python/apache/aurora/client/cli/test_options.py 21d5888c0a87e07a385159de0b56420086666d9d
src/test/python/apache/aurora/client/hooks/test_hooked_api.py 67517a23535e3f535c3114193a5ae06bf4326515
src/test/python/apache/aurora/client/hooks/test_non_hooked_api.py f4b771bc968143845772a0149a943c97c5aaf963
Diff: https://reviews.apache.org/r/43373/diff/
Testing
-------
local and manual tests in vagrant
Thanks,
Maxim Khutornenko
Re: Review Request 43373: Implementing 'aurora job add' command.
Posted by Maxim Khutornenko <ma...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43373/
-----------------------------------------------------------
(Updated Feb. 10, 2016, 6:20 p.m.)
Review request for Aurora, John Sirois and Bill Farner.
Changes
-------
John's comments.
Bugs: AURORA-1258
https://issues.apache.org/jira/browse/AURORA-1258
Repository: aurora
Description
-------
Taking advantage of the refactored addInstances RPC to add a 'scale out' client command.
The mitigation of .aurora config file `instances` value mismatch is currently discussed at http://markmail.org/message/5ekpapmxr4vxxcd5 and will be addressed separately.
Diffs (updated)
-----
src/main/python/apache/aurora/client/api/__init__.py 63bd649377816c72ef2453d9abb7363637a91680
src/main/python/apache/aurora/client/cli/context.py 24a37ec60a8845b4816d292c49fed814ba0a5854
src/main/python/apache/aurora/client/cli/jobs.py 67ab4f01eca911aea041328410c6104b63ff8344
src/main/python/apache/aurora/client/cli/options.py 226397865e54cf4c0a59ecb91b93ddc10967edf7
src/main/python/apache/aurora/client/hooks/hooked_api.py 185e57d08aa13b687529fefd7c356f3d50c6af33
src/test/python/apache/aurora/api_util.py 9d44b884e5e67506acaaecda1d9ff02a80cf1567
src/test/python/apache/aurora/client/api/test_api.py 974fc7e5a8ea13020e2c01a2088a8535947f05bc
src/test/python/apache/aurora/client/cli/test_add.py PRE-CREATION
src/test/python/apache/aurora/client/cli/test_options.py 21d5888c0a87e07a385159de0b56420086666d9d
src/test/python/apache/aurora/client/hooks/test_hooked_api.py 67517a23535e3f535c3114193a5ae06bf4326515
src/test/python/apache/aurora/client/hooks/test_non_hooked_api.py f4b771bc968143845772a0149a943c97c5aaf963
Diff: https://reviews.apache.org/r/43373/diff/
Testing
-------
local and manual tests in vagrant
Thanks,
Maxim Khutornenko
Re: Review Request 43373: Implementing 'aurora job add' command.
Posted by John Sirois <js...@apache.org>.
> On Feb. 10, 2016, 8:57 a.m., John Sirois wrote:
> > src/main/python/apache/aurora/client/cli/jobs.py, line 114
> > <https://reviews.apache.org/r/43373/diff/2/?file=1238869#file1238869line114>
> >
> > I'm not sure what the current guiding philosophy in the client is, but I'd be more comfortable with an `else: raise ValueError(...)` than a silent non-wait.
>
> John Sirois wrote:
> I guess a non-wait makes ~sense for PENDING since any job starts in this state instantaneously, so in some sense the state is always acheived - but that's pretty loose. I might actually mean wait for a currently running task to go back into PENDING state - which would not be satisfied by a no-wait no-op.
>
> Maxim Khutornenko wrote:
> This is the current behavior of the `aurora job create` and I think we should consistently apply it for the similar `aurora job add` command behavior. Having a raising `else` block will not work as you mentioned above due to all tasks being in a PENDING state on creation as default state.
> > I might actually mean wait for a currently running task to go back into PENDING state - which would not be satisfied by a no-wait no-op.
>
> I am not sure I understand this comment.
No worries, sounds like precedent wins anyhow here.
- John
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43373/#review118665
-----------------------------------------------------------
On Feb. 10, 2016, 11:20 a.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43373/
> -----------------------------------------------------------
>
> (Updated Feb. 10, 2016, 11:20 a.m.)
>
>
> Review request for Aurora, John Sirois and Bill Farner.
>
>
> Bugs: AURORA-1258
> https://issues.apache.org/jira/browse/AURORA-1258
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Taking advantage of the refactored addInstances RPC to add a 'scale out' client command.
>
> The mitigation of .aurora config file `instances` value mismatch is currently discussed at http://markmail.org/message/5ekpapmxr4vxxcd5 and will be addressed separately.
>
>
> Diffs
> -----
>
> src/main/python/apache/aurora/client/api/__init__.py 63bd649377816c72ef2453d9abb7363637a91680
> src/main/python/apache/aurora/client/cli/context.py 24a37ec60a8845b4816d292c49fed814ba0a5854
> src/main/python/apache/aurora/client/cli/jobs.py 67ab4f01eca911aea041328410c6104b63ff8344
> src/main/python/apache/aurora/client/cli/options.py 226397865e54cf4c0a59ecb91b93ddc10967edf7
> src/main/python/apache/aurora/client/hooks/hooked_api.py 185e57d08aa13b687529fefd7c356f3d50c6af33
> src/test/python/apache/aurora/api_util.py 9d44b884e5e67506acaaecda1d9ff02a80cf1567
> src/test/python/apache/aurora/client/api/test_api.py 974fc7e5a8ea13020e2c01a2088a8535947f05bc
> src/test/python/apache/aurora/client/cli/test_add.py PRE-CREATION
> src/test/python/apache/aurora/client/cli/test_options.py 21d5888c0a87e07a385159de0b56420086666d9d
> src/test/python/apache/aurora/client/hooks/test_hooked_api.py 67517a23535e3f535c3114193a5ae06bf4326515
> src/test/python/apache/aurora/client/hooks/test_non_hooked_api.py f4b771bc968143845772a0149a943c97c5aaf963
>
> Diff: https://reviews.apache.org/r/43373/diff/
>
>
> Testing
> -------
>
> local and manual tests in vagrant
>
>
> Thanks,
>
> Maxim Khutornenko
>
>
Re: Review Request 43373: Implementing 'aurora job add' command.
Posted by Maxim Khutornenko <ma...@apache.org>.
> On Feb. 10, 2016, 3:57 p.m., John Sirois wrote:
> > src/main/python/apache/aurora/client/cli/context.py, line 195
> > <https://reviews.apache.org/r/43373/diff/2/?file=1238868#file1238868line195>
> >
> > This is a lie as is the method name, this returns a set of int and not a list of ScheduledTask. Definitely update the doc but possibly also the method name to `get_active_instance_ids_or_raise`.
>
> Maxim Khutornenko wrote:
> It's not actualy a lie. We use `instances` in many other places in client (e.g. jobs.py, options.py, JobMonitor etc.) with the meaning of `instance_ids`. We use `tasks` when we mean `ScheduledTask`. I prefer to keep this convention as more concise.
>
> John Sirois wrote:
> Ok, but the "Same as ..." is a lie. The get_active_instances returns ScheduledTask, this returns int, so that bit of language could stand correction.
That I certainly agree with. Changed that method name to `get_active_tasks`.
- Maxim
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43373/#review118665
-----------------------------------------------------------
On Feb. 10, 2016, 6:20 p.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43373/
> -----------------------------------------------------------
>
> (Updated Feb. 10, 2016, 6:20 p.m.)
>
>
> Review request for Aurora, John Sirois and Bill Farner.
>
>
> Bugs: AURORA-1258
> https://issues.apache.org/jira/browse/AURORA-1258
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Taking advantage of the refactored addInstances RPC to add a 'scale out' client command.
>
> The mitigation of .aurora config file `instances` value mismatch is currently discussed at http://markmail.org/message/5ekpapmxr4vxxcd5 and will be addressed separately.
>
>
> Diffs
> -----
>
> src/main/python/apache/aurora/client/api/__init__.py 63bd649377816c72ef2453d9abb7363637a91680
> src/main/python/apache/aurora/client/cli/context.py 24a37ec60a8845b4816d292c49fed814ba0a5854
> src/main/python/apache/aurora/client/cli/jobs.py 67ab4f01eca911aea041328410c6104b63ff8344
> src/main/python/apache/aurora/client/cli/options.py 226397865e54cf4c0a59ecb91b93ddc10967edf7
> src/main/python/apache/aurora/client/hooks/hooked_api.py 185e57d08aa13b687529fefd7c356f3d50c6af33
> src/test/python/apache/aurora/api_util.py 9d44b884e5e67506acaaecda1d9ff02a80cf1567
> src/test/python/apache/aurora/client/api/test_api.py 974fc7e5a8ea13020e2c01a2088a8535947f05bc
> src/test/python/apache/aurora/client/cli/test_add.py PRE-CREATION
> src/test/python/apache/aurora/client/cli/test_options.py 21d5888c0a87e07a385159de0b56420086666d9d
> src/test/python/apache/aurora/client/hooks/test_hooked_api.py 67517a23535e3f535c3114193a5ae06bf4326515
> src/test/python/apache/aurora/client/hooks/test_non_hooked_api.py f4b771bc968143845772a0149a943c97c5aaf963
>
> Diff: https://reviews.apache.org/r/43373/diff/
>
>
> Testing
> -------
>
> local and manual tests in vagrant
>
>
> Thanks,
>
> Maxim Khutornenko
>
>
Re: Review Request 43373: Implementing 'aurora job add' command.
Posted by Maxim Khutornenko <ma...@apache.org>.
> On Feb. 10, 2016, 3:57 p.m., John Sirois wrote:
> > src/main/python/apache/aurora/client/cli/jobs.py, line 114
> > <https://reviews.apache.org/r/43373/diff/2/?file=1238869#file1238869line114>
> >
> > I'm not sure what the current guiding philosophy in the client is, but I'd be more comfortable with an `else: raise ValueError(...)` than a silent non-wait.
>
> John Sirois wrote:
> I guess a non-wait makes ~sense for PENDING since any job starts in this state instantaneously, so in some sense the state is always acheived - but that's pretty loose. I might actually mean wait for a currently running task to go back into PENDING state - which would not be satisfied by a no-wait no-op.
This is the current behavior of the `aurora job create` and I think we should consistently apply it for the similar `aurora job add` command behavior. Having a raising `else` block will not work as you mentioned above due to all tasks being in a PENDING state on creation as default state.
> I might actually mean wait for a currently running task to go back into PENDING state - which would not be satisfied by a no-wait no-op.
I am not sure I understand this comment.
> On Feb. 10, 2016, 3:57 p.m., John Sirois wrote:
> > src/main/python/apache/aurora/client/cli/options.py, line 279
> > <https://reviews.apache.org/r/43373/diff/2/?file=1238870#file1238870line279>
> >
> > The parens around the help string aren't needed to capture the line-break ws adjacent concat since there are outer parens from CommandOption(...) - not sure of the style conventions; however, other CommandOptions in this file have multiline help w/o the extra parens.
Carry over from the jobs.py where this option was originally defined. Dropped.
> On Feb. 10, 2016, 3:57 p.m., John Sirois wrote:
> > src/main/python/apache/aurora/client/cli/jobs.py, line 100
> > <https://reviews.apache.org/r/43373/diff/2/?file=1238869#file1238869line100>
> >
> > s/CREATE_INSTANCE_WAIT_OPTION/ADD_INSTANCE_WAIT_OPTION/
Good catch. Renaming leftover.
> On Feb. 10, 2016, 3:57 p.m., John Sirois wrote:
> > src/main/python/apache/aurora/client/cli/context.py, line 201
> > <https://reviews.apache.org/r/43373/diff/2/?file=1238868#file1238868line201>
> >
> > Prefer `:type list of int` - better for humans _and_ intellij - which is mart enough to know what to do with the parameterization in inspections.
Done.
> On Feb. 10, 2016, 3:57 p.m., John Sirois wrote:
> > src/main/python/apache/aurora/client/cli/context.py, line 195
> > <https://reviews.apache.org/r/43373/diff/2/?file=1238868#file1238868line195>
> >
> > This is a lie as is the method name, this returns a set of int and not a list of ScheduledTask. Definitely update the doc but possibly also the method name to `get_active_instance_ids_or_raise`.
It's not actualy a lie. We use `instances` in many other places in client (e.g. jobs.py, options.py, JobMonitor etc.) with the meaning of `instance_ids`. We use `tasks` when we mean `ScheduledTask`. I prefer to keep this convention as more concise.
> On Feb. 10, 2016, 3:57 p.m., John Sirois wrote:
> > src/main/python/apache/aurora/client/cli/context.py, line 194
> > <https://reviews.apache.org/r/43373/diff/2/?file=1238868#file1238868line194>
> >
> > Along the same lines as the rec below, consider s/instances/instance_ids/ for clarity.
Keeping `instances` as a more prevalent convention as described below.
> On Feb. 10, 2016, 3:57 p.m., John Sirois wrote:
> > src/main/python/apache/aurora/client/api/__init__.py, line 105
> > <https://reviews.apache.org/r/43373/diff/2/?file=1238867#file1238867line105>
> >
> > Instead of 'with a task config of instance 2.', 'with the task config of instance 2.' or, better I think, 'using the task config of instance 2.'
Taken your last suggestion.
- Maxim
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43373/#review118665
-----------------------------------------------------------
On Feb. 9, 2016, 10:04 p.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43373/
> -----------------------------------------------------------
>
> (Updated Feb. 9, 2016, 10:04 p.m.)
>
>
> Review request for Aurora, John Sirois and Bill Farner.
>
>
> Bugs: AURORA-1258
> https://issues.apache.org/jira/browse/AURORA-1258
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Taking advantage of the refactored addInstances RPC to add a 'scale out' client command.
>
> The mitigation of .aurora config file `instances` value mismatch is currently discussed at http://markmail.org/message/5ekpapmxr4vxxcd5 and will be addressed separately.
>
>
> Diffs
> -----
>
> src/main/python/apache/aurora/client/api/__init__.py 63bd649377816c72ef2453d9abb7363637a91680
> src/main/python/apache/aurora/client/cli/context.py 24a37ec60a8845b4816d292c49fed814ba0a5854
> src/main/python/apache/aurora/client/cli/jobs.py deba3a9026384e5bd436e611ab5b5ae3afc1cfcc
> src/main/python/apache/aurora/client/cli/options.py 226397865e54cf4c0a59ecb91b93ddc10967edf7
> src/main/python/apache/aurora/client/hooks/hooked_api.py 185e57d08aa13b687529fefd7c356f3d50c6af33
> src/test/python/apache/aurora/api_util.py 9d44b884e5e67506acaaecda1d9ff02a80cf1567
> src/test/python/apache/aurora/client/api/test_api.py 1f041f42fcd5ee5d147afacd9c8f83aa3129fe47
> src/test/python/apache/aurora/client/cli/test_add.py PRE-CREATION
> src/test/python/apache/aurora/client/cli/test_options.py 21d5888c0a87e07a385159de0b56420086666d9d
> src/test/python/apache/aurora/client/hooks/test_hooked_api.py 67517a23535e3f535c3114193a5ae06bf4326515
> src/test/python/apache/aurora/client/hooks/test_non_hooked_api.py f4b771bc968143845772a0149a943c97c5aaf963
>
> Diff: https://reviews.apache.org/r/43373/diff/
>
>
> Testing
> -------
>
> local and manual tests in vagrant
>
>
> Thanks,
>
> Maxim Khutornenko
>
>
Re: Review Request 43373: Implementing 'aurora job add' command.
Posted by John Sirois <js...@apache.org>.
> On Feb. 10, 2016, 8:57 a.m., John Sirois wrote:
> > src/main/python/apache/aurora/client/cli/context.py, line 195
> > <https://reviews.apache.org/r/43373/diff/2/?file=1238868#file1238868line195>
> >
> > This is a lie as is the method name, this returns a set of int and not a list of ScheduledTask. Definitely update the doc but possibly also the method name to `get_active_instance_ids_or_raise`.
>
> Maxim Khutornenko wrote:
> It's not actualy a lie. We use `instances` in many other places in client (e.g. jobs.py, options.py, JobMonitor etc.) with the meaning of `instance_ids`. We use `tasks` when we mean `ScheduledTask`. I prefer to keep this convention as more concise.
Ok, but the "Same as ..." is a lie. The get_active_instances returns ScheduledTask, this returns int, so that bit of language could stand correction.
- John
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43373/#review118665
-----------------------------------------------------------
On Feb. 10, 2016, 11:20 a.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43373/
> -----------------------------------------------------------
>
> (Updated Feb. 10, 2016, 11:20 a.m.)
>
>
> Review request for Aurora, John Sirois and Bill Farner.
>
>
> Bugs: AURORA-1258
> https://issues.apache.org/jira/browse/AURORA-1258
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Taking advantage of the refactored addInstances RPC to add a 'scale out' client command.
>
> The mitigation of .aurora config file `instances` value mismatch is currently discussed at http://markmail.org/message/5ekpapmxr4vxxcd5 and will be addressed separately.
>
>
> Diffs
> -----
>
> src/main/python/apache/aurora/client/api/__init__.py 63bd649377816c72ef2453d9abb7363637a91680
> src/main/python/apache/aurora/client/cli/context.py 24a37ec60a8845b4816d292c49fed814ba0a5854
> src/main/python/apache/aurora/client/cli/jobs.py 67ab4f01eca911aea041328410c6104b63ff8344
> src/main/python/apache/aurora/client/cli/options.py 226397865e54cf4c0a59ecb91b93ddc10967edf7
> src/main/python/apache/aurora/client/hooks/hooked_api.py 185e57d08aa13b687529fefd7c356f3d50c6af33
> src/test/python/apache/aurora/api_util.py 9d44b884e5e67506acaaecda1d9ff02a80cf1567
> src/test/python/apache/aurora/client/api/test_api.py 974fc7e5a8ea13020e2c01a2088a8535947f05bc
> src/test/python/apache/aurora/client/cli/test_add.py PRE-CREATION
> src/test/python/apache/aurora/client/cli/test_options.py 21d5888c0a87e07a385159de0b56420086666d9d
> src/test/python/apache/aurora/client/hooks/test_hooked_api.py 67517a23535e3f535c3114193a5ae06bf4326515
> src/test/python/apache/aurora/client/hooks/test_non_hooked_api.py f4b771bc968143845772a0149a943c97c5aaf963
>
> Diff: https://reviews.apache.org/r/43373/diff/
>
>
> Testing
> -------
>
> local and manual tests in vagrant
>
>
> Thanks,
>
> Maxim Khutornenko
>
>
Re: Review Request 43373: Implementing 'aurora job add' command.
Posted by John Sirois <js...@apache.org>.
> On Feb. 10, 2016, 8:57 a.m., John Sirois wrote:
> > src/main/python/apache/aurora/client/cli/jobs.py, line 114
> > <https://reviews.apache.org/r/43373/diff/2/?file=1238869#file1238869line114>
> >
> > I'm not sure what the current guiding philosophy in the client is, but I'd be more comfortable with an `else: raise ValueError(...)` than a silent non-wait.
I guess a non-wait makes ~sense for PENDING since any job starts in this state instantaneously, so in some sense the state is always acheived - but that's pretty loose. I might actually mean wait for a currently running task to go back into PENDING state - which would not be satisfied by a no-wait no-op.
- John
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43373/#review118665
-----------------------------------------------------------
On Feb. 9, 2016, 3:04 p.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43373/
> -----------------------------------------------------------
>
> (Updated Feb. 9, 2016, 3:04 p.m.)
>
>
> Review request for Aurora, John Sirois and Bill Farner.
>
>
> Bugs: AURORA-1258
> https://issues.apache.org/jira/browse/AURORA-1258
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Taking advantage of the refactored addInstances RPC to add a 'scale out' client command.
>
> The mitigation of .aurora config file `instances` value mismatch is currently discussed at http://markmail.org/message/5ekpapmxr4vxxcd5 and will be addressed separately.
>
>
> Diffs
> -----
>
> src/main/python/apache/aurora/client/api/__init__.py 63bd649377816c72ef2453d9abb7363637a91680
> src/main/python/apache/aurora/client/cli/context.py 24a37ec60a8845b4816d292c49fed814ba0a5854
> src/main/python/apache/aurora/client/cli/jobs.py deba3a9026384e5bd436e611ab5b5ae3afc1cfcc
> src/main/python/apache/aurora/client/cli/options.py 226397865e54cf4c0a59ecb91b93ddc10967edf7
> src/main/python/apache/aurora/client/hooks/hooked_api.py 185e57d08aa13b687529fefd7c356f3d50c6af33
> src/test/python/apache/aurora/api_util.py 9d44b884e5e67506acaaecda1d9ff02a80cf1567
> src/test/python/apache/aurora/client/api/test_api.py 1f041f42fcd5ee5d147afacd9c8f83aa3129fe47
> src/test/python/apache/aurora/client/cli/test_add.py PRE-CREATION
> src/test/python/apache/aurora/client/cli/test_options.py 21d5888c0a87e07a385159de0b56420086666d9d
> src/test/python/apache/aurora/client/hooks/test_hooked_api.py 67517a23535e3f535c3114193a5ae06bf4326515
> src/test/python/apache/aurora/client/hooks/test_non_hooked_api.py f4b771bc968143845772a0149a943c97c5aaf963
>
> Diff: https://reviews.apache.org/r/43373/diff/
>
>
> Testing
> -------
>
> local and manual tests in vagrant
>
>
> Thanks,
>
> Maxim Khutornenko
>
>
Re: Review Request 43373: Implementing 'aurora job add' command.
Posted by John Sirois <js...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43373/#review118665
-----------------------------------------------------------
Ship it!
This lgtm, all small stuff.
src/main/python/apache/aurora/client/api/__init__.py (line 105)
<https://reviews.apache.org/r/43373/#comment179958>
Instead of 'with a task config of instance 2.', 'with the task config of instance 2.' or, better I think, 'using the task config of instance 2.'
src/main/python/apache/aurora/client/cli/context.py (line 193)
<https://reviews.apache.org/r/43373/#comment179961>
Along the same lines as the rec below, consider s/instances/instance_ids/ for clarity.
src/main/python/apache/aurora/client/cli/context.py (line 194)
<https://reviews.apache.org/r/43373/#comment179959>
This is a lie as is the method name, this returns a set of int and not a list of ScheduledTask. Definitely update the doc but possibly also the method name to `get_active_instance_ids_or_raise`.
src/main/python/apache/aurora/client/cli/context.py (line 200)
<https://reviews.apache.org/r/43373/#comment179960>
Prefer `:type list of int` - better for humans _and_ intellij - which is mart enough to know what to do with the parameterization in inspections.
src/main/python/apache/aurora/client/cli/jobs.py (line 100)
<https://reviews.apache.org/r/43373/#comment179963>
s/CREATE_INSTANCE_WAIT_OPTION/ADD_INSTANCE_WAIT_OPTION/
src/main/python/apache/aurora/client/cli/jobs.py (line 114)
<https://reviews.apache.org/r/43373/#comment179962>
I'm not sure what the current guiding philosophy in the client is, but I'd be more comfortable with an `else: raise ValueError(...)` than a silent non-wait.
src/main/python/apache/aurora/client/cli/options.py (line 279)
<https://reviews.apache.org/r/43373/#comment179964>
The parens around the help string aren't needed to capture the line-break ws adjacent concat since there are outer parens from CommandOption(...) - not sure of the style conventions; however, other CommandOptions in this file have multiline help w/o the extra parens.
- John Sirois
On Feb. 9, 2016, 3:04 p.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43373/
> -----------------------------------------------------------
>
> (Updated Feb. 9, 2016, 3:04 p.m.)
>
>
> Review request for Aurora, John Sirois and Bill Farner.
>
>
> Bugs: AURORA-1258
> https://issues.apache.org/jira/browse/AURORA-1258
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Taking advantage of the refactored addInstances RPC to add a 'scale out' client command.
>
> The mitigation of .aurora config file `instances` value mismatch is currently discussed at http://markmail.org/message/5ekpapmxr4vxxcd5 and will be addressed separately.
>
>
> Diffs
> -----
>
> src/main/python/apache/aurora/client/api/__init__.py 63bd649377816c72ef2453d9abb7363637a91680
> src/main/python/apache/aurora/client/cli/context.py 24a37ec60a8845b4816d292c49fed814ba0a5854
> src/main/python/apache/aurora/client/cli/jobs.py deba3a9026384e5bd436e611ab5b5ae3afc1cfcc
> src/main/python/apache/aurora/client/cli/options.py 226397865e54cf4c0a59ecb91b93ddc10967edf7
> src/main/python/apache/aurora/client/hooks/hooked_api.py 185e57d08aa13b687529fefd7c356f3d50c6af33
> src/test/python/apache/aurora/api_util.py 9d44b884e5e67506acaaecda1d9ff02a80cf1567
> src/test/python/apache/aurora/client/api/test_api.py 1f041f42fcd5ee5d147afacd9c8f83aa3129fe47
> src/test/python/apache/aurora/client/cli/test_add.py PRE-CREATION
> src/test/python/apache/aurora/client/cli/test_options.py 21d5888c0a87e07a385159de0b56420086666d9d
> src/test/python/apache/aurora/client/hooks/test_hooked_api.py 67517a23535e3f535c3114193a5ae06bf4326515
> src/test/python/apache/aurora/client/hooks/test_non_hooked_api.py f4b771bc968143845772a0149a943c97c5aaf963
>
> Diff: https://reviews.apache.org/r/43373/diff/
>
>
> Testing
> -------
>
> local and manual tests in vagrant
>
>
> Thanks,
>
> Maxim Khutornenko
>
>
Re: Review Request 43373: Implementing 'aurora job add' command.
Posted by Maxim Khutornenko <ma...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43373/
-----------------------------------------------------------
(Updated Feb. 9, 2016, 10:04 p.m.)
Review request for Aurora, John Sirois and Bill Farner.
Changes
-------
CR feedback.
Bugs: AURORA-1258
https://issues.apache.org/jira/browse/AURORA-1258
Repository: aurora
Description
-------
Taking advantage of the refactored addInstances RPC to add a 'scale out' client command.
The mitigation of .aurora config file `instances` value mismatch is currently discussed at http://markmail.org/message/5ekpapmxr4vxxcd5 and will be addressed separately.
Diffs (updated)
-----
src/main/python/apache/aurora/client/api/__init__.py 63bd649377816c72ef2453d9abb7363637a91680
src/main/python/apache/aurora/client/cli/context.py 24a37ec60a8845b4816d292c49fed814ba0a5854
src/main/python/apache/aurora/client/cli/jobs.py deba3a9026384e5bd436e611ab5b5ae3afc1cfcc
src/main/python/apache/aurora/client/cli/options.py 226397865e54cf4c0a59ecb91b93ddc10967edf7
src/main/python/apache/aurora/client/hooks/hooked_api.py 185e57d08aa13b687529fefd7c356f3d50c6af33
src/test/python/apache/aurora/api_util.py 9d44b884e5e67506acaaecda1d9ff02a80cf1567
src/test/python/apache/aurora/client/api/test_api.py 1f041f42fcd5ee5d147afacd9c8f83aa3129fe47
src/test/python/apache/aurora/client/cli/test_add.py PRE-CREATION
src/test/python/apache/aurora/client/cli/test_options.py 21d5888c0a87e07a385159de0b56420086666d9d
src/test/python/apache/aurora/client/hooks/test_hooked_api.py 67517a23535e3f535c3114193a5ae06bf4326515
src/test/python/apache/aurora/client/hooks/test_non_hooked_api.py f4b771bc968143845772a0149a943c97c5aaf963
Diff: https://reviews.apache.org/r/43373/diff/
Testing
-------
local and manual tests in vagrant
Thanks,
Maxim Khutornenko
Re: Review Request 43373: Implementing 'aurora job add' command.
Posted by Aurora ReviewBot <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43373/#review118458
-----------------------------------------------------------
Ship it!
Master (dd79934) is green with this patch.
./build-support/jenkins/build.sh
I will refresh this build result if you post a review containing "@ReviewBot retry"
- Aurora ReviewBot
On Feb. 9, 2016, 7:44 p.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43373/
> -----------------------------------------------------------
>
> (Updated Feb. 9, 2016, 7:44 p.m.)
>
>
> Review request for Aurora, John Sirois and Bill Farner.
>
>
> Bugs: AURORA-1258
> https://issues.apache.org/jira/browse/AURORA-1258
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Taking advantage of the refactored addInstances RPC to add a 'scale out' client command.
>
> The mitigation of .aurora config file `instances` value mismatch is currently discussed at http://markmail.org/message/5ekpapmxr4vxxcd5 and will be addressed separately.
>
>
> Diffs
> -----
>
> src/main/python/apache/aurora/client/api/__init__.py 63bd649377816c72ef2453d9abb7363637a91680
> src/main/python/apache/aurora/client/cli/context.py 24a37ec60a8845b4816d292c49fed814ba0a5854
> src/main/python/apache/aurora/client/cli/jobs.py deba3a9026384e5bd436e611ab5b5ae3afc1cfcc
> src/main/python/apache/aurora/client/cli/options.py 226397865e54cf4c0a59ecb91b93ddc10967edf7
> src/main/python/apache/aurora/client/hooks/hooked_api.py 185e57d08aa13b687529fefd7c356f3d50c6af33
> src/test/python/apache/aurora/api_util.py 9d44b884e5e67506acaaecda1d9ff02a80cf1567
> src/test/python/apache/aurora/client/api/test_api.py 1f041f42fcd5ee5d147afacd9c8f83aa3129fe47
> src/test/python/apache/aurora/client/cli/test_add.py PRE-CREATION
> src/test/python/apache/aurora/client/hooks/test_hooked_api.py 67517a23535e3f535c3114193a5ae06bf4326515
> src/test/python/apache/aurora/client/hooks/test_non_hooked_api.py f4b771bc968143845772a0149a943c97c5aaf963
>
> Diff: https://reviews.apache.org/r/43373/diff/
>
>
> Testing
> -------
>
> local and manual tests in vagrant
>
>
> Thanks,
>
> Maxim Khutornenko
>
>
Re: Review Request 43373: Implementing 'aurora job add' command.
Posted by Maxim Khutornenko <ma...@apache.org>.
> On Feb. 9, 2016, 9:11 p.m., Joshua Cohen wrote:
> > src/main/python/apache/aurora/client/cli/context.py, line 75
> > <https://reviews.apache.org/r/43373/diff/1/?file=1238720#file1238720line75>
> >
> > s/an/the
Done.
> On Feb. 9, 2016, 9:11 p.m., Joshua Cohen wrote:
> > src/main/python/apache/aurora/client/cli/context.py, line 196
> > <https://reviews.apache.org/r/43373/diff/1/?file=1238720#file1238720line196>
> >
> > s/is/are
Done.
> On Feb. 9, 2016, 9:11 p.m., Joshua Cohen wrote:
> > src/main/python/apache/aurora/client/cli/jobs.py, line 471
> > <https://reviews.apache.org/r/43373/diff/1/?file=1238721#file1238721line471>
> >
> > What happens if someone specifies an instance range here instead of a single instance?
This is exactly the reason I used TASK_INSTANCE_ARGUMENT instead of INSTANCE_SPEC_ARGUMENT, which disallows instance ranges.
However, your question prompted me to take a look at the error message and it wasn't ideal:
```
aurora job add: error: argument task_instance: invalid parse_task_instance_key value: 'devcluster/www-data/prod/hello/2-4'
```
Further look uncovered a few places in options.py parsers where we raise ValueError instead of ArgumentTypeError. Fixed all of relevant cases and added tests. The new message is much better now:
```
aurora job add: error: argument task_instance: Instance must be an integer, but got 2-4
```
- Maxim
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43373/#review118464
-----------------------------------------------------------
On Feb. 9, 2016, 10:04 p.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43373/
> -----------------------------------------------------------
>
> (Updated Feb. 9, 2016, 10:04 p.m.)
>
>
> Review request for Aurora, John Sirois and Bill Farner.
>
>
> Bugs: AURORA-1258
> https://issues.apache.org/jira/browse/AURORA-1258
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Taking advantage of the refactored addInstances RPC to add a 'scale out' client command.
>
> The mitigation of .aurora config file `instances` value mismatch is currently discussed at http://markmail.org/message/5ekpapmxr4vxxcd5 and will be addressed separately.
>
>
> Diffs
> -----
>
> src/main/python/apache/aurora/client/api/__init__.py 63bd649377816c72ef2453d9abb7363637a91680
> src/main/python/apache/aurora/client/cli/context.py 24a37ec60a8845b4816d292c49fed814ba0a5854
> src/main/python/apache/aurora/client/cli/jobs.py deba3a9026384e5bd436e611ab5b5ae3afc1cfcc
> src/main/python/apache/aurora/client/cli/options.py 226397865e54cf4c0a59ecb91b93ddc10967edf7
> src/main/python/apache/aurora/client/hooks/hooked_api.py 185e57d08aa13b687529fefd7c356f3d50c6af33
> src/test/python/apache/aurora/api_util.py 9d44b884e5e67506acaaecda1d9ff02a80cf1567
> src/test/python/apache/aurora/client/api/test_api.py 1f041f42fcd5ee5d147afacd9c8f83aa3129fe47
> src/test/python/apache/aurora/client/cli/test_add.py PRE-CREATION
> src/test/python/apache/aurora/client/cli/test_options.py 21d5888c0a87e07a385159de0b56420086666d9d
> src/test/python/apache/aurora/client/hooks/test_hooked_api.py 67517a23535e3f535c3114193a5ae06bf4326515
> src/test/python/apache/aurora/client/hooks/test_non_hooked_api.py f4b771bc968143845772a0149a943c97c5aaf963
>
> Diff: https://reviews.apache.org/r/43373/diff/
>
>
> Testing
> -------
>
> local and manual tests in vagrant
>
>
> Thanks,
>
> Maxim Khutornenko
>
>
Re: Review Request 43373: Implementing 'aurora job add' command.
Posted by Joshua Cohen <jc...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43373/#review118464
-----------------------------------------------------------
src/main/python/apache/aurora/client/cli/context.py (line 75)
<https://reviews.apache.org/r/43373/#comment179694>
s/an/the
src/main/python/apache/aurora/client/cli/context.py (line 195)
<https://reviews.apache.org/r/43373/#comment179695>
s/is/are
src/main/python/apache/aurora/client/cli/jobs.py (line 465)
<https://reviews.apache.org/r/43373/#comment179696>
What happens if someone specifies an instance range here instead of a single instance?
- Joshua Cohen
On Feb. 9, 2016, 7:44 p.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43373/
> -----------------------------------------------------------
>
> (Updated Feb. 9, 2016, 7:44 p.m.)
>
>
> Review request for Aurora, John Sirois and Bill Farner.
>
>
> Bugs: AURORA-1258
> https://issues.apache.org/jira/browse/AURORA-1258
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Taking advantage of the refactored addInstances RPC to add a 'scale out' client command.
>
> The mitigation of .aurora config file `instances` value mismatch is currently discussed at http://markmail.org/message/5ekpapmxr4vxxcd5 and will be addressed separately.
>
>
> Diffs
> -----
>
> src/main/python/apache/aurora/client/api/__init__.py 63bd649377816c72ef2453d9abb7363637a91680
> src/main/python/apache/aurora/client/cli/context.py 24a37ec60a8845b4816d292c49fed814ba0a5854
> src/main/python/apache/aurora/client/cli/jobs.py deba3a9026384e5bd436e611ab5b5ae3afc1cfcc
> src/main/python/apache/aurora/client/cli/options.py 226397865e54cf4c0a59ecb91b93ddc10967edf7
> src/main/python/apache/aurora/client/hooks/hooked_api.py 185e57d08aa13b687529fefd7c356f3d50c6af33
> src/test/python/apache/aurora/api_util.py 9d44b884e5e67506acaaecda1d9ff02a80cf1567
> src/test/python/apache/aurora/client/api/test_api.py 1f041f42fcd5ee5d147afacd9c8f83aa3129fe47
> src/test/python/apache/aurora/client/cli/test_add.py PRE-CREATION
> src/test/python/apache/aurora/client/hooks/test_hooked_api.py 67517a23535e3f535c3114193a5ae06bf4326515
> src/test/python/apache/aurora/client/hooks/test_non_hooked_api.py f4b771bc968143845772a0149a943c97c5aaf963
>
> Diff: https://reviews.apache.org/r/43373/diff/
>
>
> Testing
> -------
>
> local and manual tests in vagrant
>
>
> Thanks,
>
> Maxim Khutornenko
>
>