Re: Review Request 19061: Disable kill of production jobs without force flag
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/19061/ --- (Updated March 12, 2014, 2:12 p.m.) Review request for Aurora, Kevin Sweeney and Maxim Khutornenko. Changes --- Address reviews. Repository: aurora Description --- Disable kill of production jobs without force flag. Diffs (updated) - src/main/python/apache/aurora/client/commands/core.py ff0f1f8668c8c405fa3a41b70cae32004034e223 src/test/python/apache/aurora/client/commands/test_kill.py 7639dc98bfea0663461d15e3d46f1aedd13b124f Diff: https://reviews.apache.org/r/19061/diff/ Testing --- Added new test cases to the unit test for the kill command to cover the new cases. All tests, new and old, pass. [sun-wukong incubator-aurora (kill-force)]$ ./pants src/test/python/apache/aurora/client/commands:all Build operating on targets: OrderedSet([PythonTestSuite(src/test/python/apache/aurora/client/commands/BUILD:all)]) = test session starts = platform darwin -- Python 2.6.8 -- py-1.4.20 -- pytest-2.5.2 collected 13 items src/test/python/apache/aurora/client/commands/test_admin_sla.py . == 13 passed in 0.47 seconds == = test session starts = platform darwin -- Python 2.6.8 -- py-1.4.20 -- pytest-2.5.2 collected 26 items src/test/python/apache/aurora/client/commands/test_cancel_update.py .. src/test/python/apache/aurora/client/commands/test_create.py .. src/test/python/apache/aurora/client/commands/test_diff.py ... src/test/python/apache/aurora/client/commands/test_kill.py . src/test/python/apache/aurora/client/commands/test_listjobs.py .. src/test/python/apache/aurora/client/commands/test_restart.py ... src/test/python/apache/aurora/client/commands/test_status.py .. src/test/python/apache/aurora/client/commands/test_update.py ... == 26 passed in 1.35 seconds == = test session starts = platform darwin -- Python 2.6.8 -- py-1.4.20 -- pytest-2.5.2 collected 4 items src/test/python/apache/aurora/client/commands/test_maintenance.py == 4 passed in 0.43 seconds === = test session starts = platform darwin -- Python 2.6.8 -- py-1.4.20 -- pytest-2.5.2 collected 1 items src/test/python/apache/aurora/client/commands/test_run.py . == 1 passed in 0.49 seconds === = test session starts = platform darwin -- Python 2.6.8 -- py-1.4.20 -- pytest-2.5.2 collected 1 items src/test/python/apache/aurora/client/commands/test_ssh.py . == 1 passed in 0.54 seconds === src.test.python.apache.aurora.client.commands.admin . SUCCESS src.test.python.apache.aurora.client.commands.core . SUCCESS src.test.python.apache.aurora.client.commands.maintenance . SUCCESS src.test.python.apache.aurora.client.commands.run . SUCCESS src.test.python.apache.aurora.client.commands.ssh . SUCCESS Thanks, Mark Chu-Carroll
Review Request 19143: Catch errors thrown by authentication modules.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/19143/ --- Review request for Aurora, Kevin Sweeney and Brian Wickman. Bugs: aurora-259 https://issues.apache.org/jira/browse/aurora-259 Repository: aurora Description --- Catch errors thrown by authentication modules. Authentication modules can throw arbitrary errors - ldap exceptions, password errors, etc, if auth fails with one of these app specific errors, the client current dumps its cookies in a stack dump. This change catches the error cleanly, and transforms it into a handled authentication exception. Diffs - src/main/python/apache/aurora/client/api/scheduler_client.py f770df7a23779f919cd11cc28b2aaf7cfdf9c5a1 Diff: https://reviews.apache.org/r/19143/diff/ Testing --- Thanks, Mark Chu-Carroll
Re: Review Request 18979: Add an updated version of the clientv2 doc to apache.
On March 11, 2014, 3:16 p.m., Tom Galloway wrote: Line 148: Each noun will be separate component - Each noun will be a separate component noun verb syntax; are you wedded to this? aurora get quota and aurora create job are more English (if Tarzan sounding) like than aurora quota get and aurora job create and just seem to read better as verb noun to me. Line 200: * `submit jobkey config`: submits a job to a cluster, launching the jobs task. I'm not clear here on whether this should be job's tasks, job's task, or task named 'jobs' (for the last, I'm thinking going through the jobs list in the config file, could be a task, although a bit different from the defined ones.) Line 205: they'llbe - they'll be In the noun sections, I'd strongly suggest including at least the noun, and possibly aurora noun in the examples for clarity since you've made a big deal of the noun/verb syntax thing. It took me a minute to realize submit jobkey config was short for aurora job submit jobkey config Line 217-218: cluster/role, all jobs running under the role will be listed. I'd add on the cluster between running and under the role for claity Line 224: The cron commands all manipulate cron schedule entries Maybe just me, but I'd change this to All cron commands manipulate...; initially I was reading this as The cron commands all... with commands as a verb. Line 227: Again probably just me, but schedule a job to run by cron. has me doing flashbacks to Conan the Barbarian going By Crom! and is a bit awkward. How about schedule a cron-run job Line 234: Quotas are a data object maintained by the scheduler that specifies the maximum resources that may be consumed by jobs owned by a particular role. Definitely change Quotas are a data object to A quota is a data object. I'd suggest breaking up this four chained concepts sentence info multiples; A quota is a data object maintained by the scheduler. It specifies the maximum resources that jobs owned by a particular role may consume. Maybe also emphasize that it does *not* work on a single job as specified by a jobkey, but on all jobs sharing the specified cluster and role values. Line 247: command-line - command line Line 323: remove extra whitespace. Lines 326 and 328: command-line - command line Line 331: When commands are executed - When commands execute Lines 331-2: When commands are executed, they're given an instance of a *context object*. The context object must be an instance of a subclass of `AuroraCommandContext`. This is confusing, as you seem to be using object and instance interchangably. It seems that the command gets an instance of an instance? Two things I'd suggest mentioning; 1) Is every current open source command line command and option going to be available in some way out of the box with the new client? In other words, is there any existing functionality being removed (or, for that matter, added) in this new client? 2) Will the new client still have the ability to add hooks to the existing commands? I'd think so, since as the hooks stuff operates on the underlying Aurora API which the client commands call. 3) Not for open source, but will the Twitter-internal aurora packer and aurora deploy commands still be implemented in the new client? Fixed most of this. For the remainder: - scheduling a job to run by cron is (IME) the common phrasing. - we are committed to noun/verb order. That got nailed down during design reviews. - all of the functionality of the v1 client will, at least eventually, be supported by v2. Not necessarily every option in exactly the same way - but equivalent functionality. I think that that's beyond the scope of this document. - Hooks work exactly the way that they do in the existing client - no changes. - We want to keep discussions of twitter-internal stuff out of apache - it just isn't relevant here. (But for a quick answer, yes.) - Mark --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/18979/#review36827 --- On March 11, 2014, 12:34 p.m., Mark Chu-Carroll wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/18979/ --- (Updated March 11, 2014, 12:34 p.m.) Review request for Aurora, Dave Lester and Tom Galloway. Bugs: aurora-253 https://issues.apache.org/jira/browse/aurora-253 Repository: aurora Description --- Add an updated version of the clientv2 doc to apache. (Also make a few changes in the interfaces to match the updated doc.) Diffs -
Re: Review Request 18979: Add an updated version of the clientv2 doc to apache.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/18979/ --- (Updated March 12, 2014, 5:44 p.m.) Review request for Aurora, Dave Lester and Tom Galloway. Changes --- review changes. Bugs: aurora-253 https://issues.apache.org/jira/browse/aurora-253 Repository: aurora Description --- Add an updated version of the clientv2 doc to apache. (Also make a few changes in the interfaces to match the updated doc.) Diffs (updated) - docs/clientv2.md PRE-CREATION src/main/python/apache/aurora/client/cli/__init__.py 4a6a7eef7b781be79a3d40776a3bd6f0c6e8c4c0 src/main/python/apache/aurora/client/cli/jobs.py 3b327df5f9c5f1d5e7e68863191313921a8dde44 Diff: https://reviews.apache.org/r/18979/diff/ Testing --- n/a Thanks, Mark Chu-Carroll
Re: Review Request 19143: Catch errors thrown by authentication modules.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/19143/#review36996 --- src/main/python/apache/aurora/client/api/scheduler_client.py https://reviews.apache.org/r/19143/#comment68245 I'm pretty wary of swallowing exceptions like this - seems like an easy way to paper over bugs / hide truly exceptional events. I'd at least log the traceback at debug level here so that it's available in debug output. - Kevin Sweeney On March 12, 2014, 12:57 p.m., Mark Chu-Carroll wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/19143/ --- (Updated March 12, 2014, 12:57 p.m.) Review request for Aurora, Kevin Sweeney and Brian Wickman. Bugs: aurora-259 https://issues.apache.org/jira/browse/aurora-259 Repository: aurora Description --- Catch errors thrown by authentication modules. Authentication modules can throw arbitrary errors - ldap exceptions, password errors, etc, if auth fails with one of these app specific errors, the client current dumps its cookies in a stack dump. This change catches the error cleanly, and transforms it into a handled authentication exception. Diffs - src/main/python/apache/aurora/client/api/scheduler_client.py f770df7a23779f919cd11cc28b2aaf7cfdf9c5a1 Diff: https://reviews.apache.org/r/19143/diff/ Testing --- Thanks, Mark Chu-Carroll
Re: Review Request 19159: Add killall.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/19159/#review37005 --- src/main/python/apache/aurora/client/commands/core.py https://reviews.apache.org/r/19159/#comment68251 As it's implemented, the --force flag is a required option for this command. Feels redundant as killall mandatory wait period should be enough here. I presume making kill require --shards is going to be tracked in a separate RB? - Maxim Khutornenko On March 12, 2014, 10:11 p.m., Mark Chu-Carroll wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/19159/ --- (Updated March 12, 2014, 10:11 p.m.) Review request for Aurora, Kevin Sweeney and Bill Farner. Bugs: aurora-260 https://issues.apache.org/jira/browse/aurora-260 Repository: aurora Description --- Add killall. - the kill command now requires a shards parameter. - the new killall command only works when run with --force. - killall generates a scary warning message, and pauses to give the user a chance to abort. Diffs - src/main/python/apache/aurora/client/commands/core.py ff0f1f8668c8c405fa3a41b70cae32004034e223 src/test/python/apache/aurora/client/commands/test_kill.py 7639dc98bfea0663461d15e3d46f1aedd13b124f Diff: https://reviews.apache.org/r/19159/diff/ Testing --- Modified the existing kill command's test suite, adding new tests of the new functionality. All pass. [sun-wukong incubator-aurora (killall)]$ ./pants src/test/python/apache/aurora/client/commands:core Build operating on targets: OrderedSet([PythonTests(src/test/python/apache/aurora/client/commands/BUILD:core)]) = test session starts = platform darwin -- Python 2.6.8 -- py-1.4.20 -- pytest-2.5.2 collected 26 items src/test/python/apache/aurora/client/commands/test_cancel_update.py .. src/test/python/apache/aurora/client/commands/test_create.py .. src/test/python/apache/aurora/client/commands/test_diff.py ... src/test/python/apache/aurora/client/commands/test_kill.py . src/test/python/apache/aurora/client/commands/test_listjobs.py .. src/test/python/apache/aurora/client/commands/test_restart.py ... src/test/python/apache/aurora/client/commands/test_status.py .. src/test/python/apache/aurora/client/commands/test_update.py ... = 26 passed in 11.34 seconds == src.test.python.apache.aurora.client.commands.core . SUCCESS Thanks, Mark Chu-Carroll
Re: Review Request 19159: Add killall.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/19159/#review37008 --- src/main/python/apache/aurora/client/commands/core.py https://reviews.apache.org/r/19159/#comment68255 This is deliberate: the kill command doesn't have a force option. So this ensures that there's more than a search-and-replace killall for kill: you need to deliberately use the killall command, and specify the force option. The require --shards is also in this change - see the change above in kill. - Mark Chu-Carroll On March 12, 2014, 6:11 p.m., Mark Chu-Carroll wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/19159/ --- (Updated March 12, 2014, 6:11 p.m.) Review request for Aurora, Kevin Sweeney and Bill Farner. Bugs: aurora-260 https://issues.apache.org/jira/browse/aurora-260 Repository: aurora Description --- Add killall. - the kill command now requires a shards parameter. - the new killall command only works when run with --force. - killall generates a scary warning message, and pauses to give the user a chance to abort. Diffs - src/main/python/apache/aurora/client/commands/core.py ff0f1f8668c8c405fa3a41b70cae32004034e223 src/test/python/apache/aurora/client/commands/test_kill.py 7639dc98bfea0663461d15e3d46f1aedd13b124f Diff: https://reviews.apache.org/r/19159/diff/ Testing --- Modified the existing kill command's test suite, adding new tests of the new functionality. All pass. [sun-wukong incubator-aurora (killall)]$ ./pants src/test/python/apache/aurora/client/commands:core Build operating on targets: OrderedSet([PythonTests(src/test/python/apache/aurora/client/commands/BUILD:core)]) = test session starts = platform darwin -- Python 2.6.8 -- py-1.4.20 -- pytest-2.5.2 collected 26 items src/test/python/apache/aurora/client/commands/test_cancel_update.py .. src/test/python/apache/aurora/client/commands/test_create.py .. src/test/python/apache/aurora/client/commands/test_diff.py ... src/test/python/apache/aurora/client/commands/test_kill.py . src/test/python/apache/aurora/client/commands/test_listjobs.py .. src/test/python/apache/aurora/client/commands/test_restart.py ... src/test/python/apache/aurora/client/commands/test_status.py .. src/test/python/apache/aurora/client/commands/test_update.py ... = 26 passed in 11.34 seconds == src.test.python.apache.aurora.client.commands.core . SUCCESS Thanks, Mark Chu-Carroll
Re: Review Request 19159: Add killall.
On March 12, 2014, 10:25 p.m., Mark Chu-Carroll wrote: src/main/python/apache/aurora/client/commands/core.py, line 399 https://reviews.apache.org/r/19159/diff/1/?file=517771#file517771line399 This is deliberate: the kill command doesn't have a force option. So this ensures that there's more than a search-and-replace killall for kill: you need to deliberately use the killall command, and specify the force option. The require --shards is also in this change - see the change above in kill. Missed the --shards part, thanks. - Maxim --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/19159/#review37008 --- On March 12, 2014, 10:11 p.m., Mark Chu-Carroll wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/19159/ --- (Updated March 12, 2014, 10:11 p.m.) Review request for Aurora, Kevin Sweeney and Bill Farner. Bugs: aurora-260 https://issues.apache.org/jira/browse/aurora-260 Repository: aurora Description --- Add killall. - the kill command now requires a shards parameter. - the new killall command only works when run with --force. - killall generates a scary warning message, and pauses to give the user a chance to abort. Diffs - src/main/python/apache/aurora/client/commands/core.py ff0f1f8668c8c405fa3a41b70cae32004034e223 src/test/python/apache/aurora/client/commands/test_kill.py 7639dc98bfea0663461d15e3d46f1aedd13b124f Diff: https://reviews.apache.org/r/19159/diff/ Testing --- Modified the existing kill command's test suite, adding new tests of the new functionality. All pass. [sun-wukong incubator-aurora (killall)]$ ./pants src/test/python/apache/aurora/client/commands:core Build operating on targets: OrderedSet([PythonTests(src/test/python/apache/aurora/client/commands/BUILD:core)]) = test session starts = platform darwin -- Python 2.6.8 -- py-1.4.20 -- pytest-2.5.2 collected 26 items src/test/python/apache/aurora/client/commands/test_cancel_update.py .. src/test/python/apache/aurora/client/commands/test_create.py .. src/test/python/apache/aurora/client/commands/test_diff.py ... src/test/python/apache/aurora/client/commands/test_kill.py . src/test/python/apache/aurora/client/commands/test_listjobs.py .. src/test/python/apache/aurora/client/commands/test_restart.py ... src/test/python/apache/aurora/client/commands/test_status.py .. src/test/python/apache/aurora/client/commands/test_update.py ... = 26 passed in 11.34 seconds == src.test.python.apache.aurora.client.commands.core . SUCCESS Thanks, Mark Chu-Carroll
Re: Review Request 19143: Catch errors thrown by authentication modules.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/19143/ --- (Updated March 12, 2014, 6:59 p.m.) Review request for Aurora, Kevin Sweeney and Brian Wickman. Changes --- Added debug logging of stack traceback for auth errors. Bugs: aurora-259 https://issues.apache.org/jira/browse/aurora-259 Repository: aurora Description --- Catch errors thrown by authentication modules. Authentication modules can throw arbitrary errors - ldap exceptions, password errors, etc, if auth fails with one of these app specific errors, the client current dumps its cookies in a stack dump. This change catches the error cleanly, and transforms it into a handled authentication exception. Diffs (updated) - src/main/python/apache/aurora/client/api/scheduler_client.py f770df7a23779f919cd11cc28b2aaf7cfdf9c5a1 Diff: https://reviews.apache.org/r/19143/diff/ Testing --- Thanks, Mark Chu-Carroll
