Re: Review Request 25255: Implement server-driven update commands.
On Sept. 3, 2014, 3:58 p.m., Maxim Khutornenko wrote: src/main/python/apache/aurora/client/cli/jobs.py, line 667 https://reviews.apache.org/r/25255/diff/1/?file=673959#file673959line667 How about a BROWSER_OPTION for all update commands (start/pause/resume/abort)? It will, eventually, but we don't have a set URL for it yet. That can be added when the UI is more locked down. On Sept. 3, 2014, 3:58 p.m., Maxim Khutornenko wrote: src/main/python/apache/aurora/client/cli/jobs.py, lines 704-706 https://reviews.apache.org/r/25255/diff/1/?file=673959#file673959line704 Is this a result of sharing the verb definition with start_update? Any chance to avoid sharing the option set here? With the way that the noun/verb framework works right now, no, there's not really any good way. There are three choices: (1) Have an action parameter as a selector (what this change does); (2) Have a collection of verbs, update_start, update_pause, update_resume, update_abort. (3) Add a noun for an in-progress update, in which case the commands become aurora update start, aurora update pause, etc. I really hate (2), and I've been going back and forth between (1) and (3). - Mark --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25255/#review52220 --- On Sept. 2, 2014, 12:36 p.m., Mark Chu-Carroll wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25255/ --- (Updated Sept. 2, 2014, 12:36 p.m.) Review request for Aurora. Repository: aurora Description --- This change contains the basic commands needed to work with the scheduler-driven updater. (It does not yet cover querying for the status of the update; that will come in a subsequent change.) Diffs - src/main/python/apache/aurora/client/cli/context.py 51c7d24dca664e476e62f1864d095416dfab70e4 src/main/python/apache/aurora/client/cli/jobs.py ebc22aaa5a8aed311897b3ce9632b6f7175b6080 src/test/python/apache/aurora/client/cli/BUILD e1f9ebf96774b8f5c75de8570c6ba87d953ab649 src/test/python/apache/aurora/client/cli/test_restart.py a1e7a5a94a2d336239df98e2600658b97c546901 src/test/python/apache/aurora/client/cli/test_supdate.py PRE-CREATION src/test/python/apache/aurora/client/cli/util.py 95a2123e127c9811fd2305e71cfc5c7c4376f904 Diff: https://reviews.apache.org/r/25255/diff/ Testing --- New suite of tests for the new command; all unit tests pass. Thanks, Mark Chu-Carroll
Re: Review Request 25255: Implement server-driven update commands.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25255/#review53030 --- I was about to ping about this, when I discovered that my replies to the comments never got sent - guessing chrome timed out, and I didn't notice. Please don't re-review this yet - I'm reworking some of the code; I'll send an update when it's ready. - Mark Chu-Carroll On Sept. 2, 2014, 12:36 p.m., Mark Chu-Carroll wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25255/ --- (Updated Sept. 2, 2014, 12:36 p.m.) Review request for Aurora. Repository: aurora Description --- This change contains the basic commands needed to work with the scheduler-driven updater. (It does not yet cover querying for the status of the update; that will come in a subsequent change.) Diffs - src/main/python/apache/aurora/client/cli/context.py 51c7d24dca664e476e62f1864d095416dfab70e4 src/main/python/apache/aurora/client/cli/jobs.py ebc22aaa5a8aed311897b3ce9632b6f7175b6080 src/test/python/apache/aurora/client/cli/BUILD e1f9ebf96774b8f5c75de8570c6ba87d953ab649 src/test/python/apache/aurora/client/cli/test_restart.py a1e7a5a94a2d336239df98e2600658b97c546901 src/test/python/apache/aurora/client/cli/test_supdate.py PRE-CREATION src/test/python/apache/aurora/client/cli/util.py 95a2123e127c9811fd2305e71cfc5c7c4376f904 Diff: https://reviews.apache.org/r/25255/diff/ Testing --- New suite of tests for the new command; all unit tests pass. Thanks, Mark Chu-Carroll
Review Request 25543: Update to pants 0.0.23.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25543/ --- Review request for Aurora, Joe Smith and Brian Wickman. Bugs: aurora-695 https://issues.apache.org/jira/browse/aurora-695 Repository: aurora Description --- - Update build files for the new syntax, which no longer requires 'pants(...)' around target names. - Remove no-longer-supported timeout from python_tests. Diffs - pants 4f9c351888afa1a779415730240093c3dee25dfb src/main/python/apache/aurora/admin/BUILD 7a100d1a4a74aae034082f34db051c9cc31f8540 src/main/python/apache/aurora/client/BUILD bf196bf86b36db0d72f8e096260c9a900f74d07c src/main/python/apache/aurora/client/api/BUILD 70ad38e34f14c6d54b71c8f4b2138f085658110e src/main/python/apache/aurora/client/bin/BUILD 43d747956df0611b0880f64df9955d5f5806901c src/main/python/apache/aurora/client/cli/BUILD ebe681a0d1735b7cc695dc3b7a14c4292d87ae32 src/main/python/apache/aurora/client/commands/BUILD cc16923909a7b26b0f3ac0b47bb37dafdbbf502e src/main/python/apache/aurora/client/hooks/BUILD 9471c4cba5175296030747301e246a65a39aa203 src/main/python/apache/aurora/common/BUILD b879b15127d6691b35880074fd0ceacd866a61ed src/main/python/apache/aurora/common/auth/BUILD 7e96cb2258711b2e2925d18ad9435fa986e86bca src/main/python/apache/aurora/config/BUILD 4f8fad80114ddabac8b25f70bba00119228ec675 src/main/python/apache/aurora/config/schema/BUILD 69d60aebd2a9aa353497406ae578a9997323b07e src/main/python/apache/aurora/executor/BUILD 1ad8f82cdce85cf228c53e088171918e36ed536d src/main/python/apache/aurora/executor/bin/BUILD aeb8aee6f50a0d89714626e933699c0a13b363d9 src/main/python/apache/aurora/executor/common/BUILD 335ebc4809096c5f128846cd846d33910a777968 src/main/python/apache/thermos/BUILD 0dc035f759dd9949997f0c979b3556a350cf8df7 src/main/python/apache/thermos/bin/BUILD 669f9930a3590184dc0f8b5c15c36168e715eb03 src/main/python/apache/thermos/common/BUILD 6015f9e9a23f71bf6dede1f4698fe63dbb4dcfaa src/main/python/apache/thermos/config/BUILD 0531f92ea569ffe36817b645a17fab7a712e5897 src/main/python/apache/thermos/core/BUILD 0d1d339d55ee6a569297614ac734661e5caf7ea4 src/main/python/apache/thermos/monitoring/BUILD 79da0d5cef9436d4a3d83075910decfc93e422a6 src/main/python/apache/thermos/observer/BUILD 49b844ffc1b1d5911fc28d14294d088c3d0b6e4b src/main/python/apache/thermos/observer/bin/BUILD 044ca66b18282daf17a4198ff369d954e14c9b6d src/main/python/apache/thermos/observer/http/BUILD 901ad9c61e4dd1c61f5fbf4467becb8c881a64ed src/main/python/apache/thermos/testing/BUILD dc328a63788381307576b5a43ecdc704bb764473 src/main/thrift/org/apache/aurora/gen/BUILD 947504ec1f9582496952b23e66d7f5f20a168ce7 src/test/python/BUILD f01efff2e4982a475221b5739dfe1e8fd1a41d92 src/test/python/apache/aurora/BUILD 6555b984a713ef786aef5688b206ae9d8017c48d src/test/python/apache/aurora/admin/BUILD 5e170d6c15a95e2511b69e18a255d7364c2e7a4d src/test/python/apache/aurora/client/BUILD 831a72d39b27ca2aca466a38914bbf40ff94 src/test/python/apache/aurora/client/api/BUILD b4f08c6192e6bf6b38665197e98db7235751ae86 src/test/python/apache/aurora/client/cli/BUILD e1f9ebf96774b8f5c75de8570c6ba87d953ab649 src/test/python/apache/aurora/client/commands/BUILD 17933dedfa08c9d12c369087bf801e7c35cdde9b src/test/python/apache/aurora/client/hooks/BUILD f7856a2d5dc7e5d1edc480f64d5db97d88c71b70 src/test/python/apache/aurora/common/BUILD e949ba8859d5567c62623bec9d5ba86a8463fbaa src/test/python/apache/aurora/config/BUILD 37bbd27e13a2a3589faff7285f04e3c44ca57eeb src/test/python/apache/aurora/executor/BUILD 4d43e256ad131223cc1ac36a406d42a979a8a2dd src/test/python/apache/aurora/executor/common/BUILD 7d8934046b56ac2c0c16440cfc571dc370767a14 src/test/python/apache/thermos/BUILD cb93a4622e33ef96855b89a7c138f42033368950 src/test/python/apache/thermos/bin/BUILD 4b59f3879298de9664f168150ea9029e013e7913 src/test/python/apache/thermos/common/BUILD 36fa6a69b5e77a645a65c52fef6ec9343bf541bc src/test/python/apache/thermos/config/BUILD 42445ceccba8dfe8296a22a174aca6123bdfdb52 src/test/python/apache/thermos/core/BUILD 8f5c626c2e89834dbb4938c3c69ef8c79558e12b src/test/python/apache/thermos/monitoring/BUILD ea4005b52be3185e553f7d23fb29b89f68befa50 Diff: https://reviews.apache.org/r/25543/diff/ Testing --- - Ran all unit tests: several fail, but they also fail under the previous version of pants. - Built all python_binary targets in src/main/python/apache/aurora. - Verified that resulting pexes executed correctly. Thanks, Mark Chu-Carroll
Re: Review Request 25543: Update to pants 0.0.23.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25543/#review53048 --- Ship it! Ship It! - Zameer Manji On Sept. 11, 2014, 9:13 a.m., Mark Chu-Carroll wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25543/ --- (Updated Sept. 11, 2014, 9:13 a.m.) Review request for Aurora, Joe Smith and Brian Wickman. Bugs: aurora-695 https://issues.apache.org/jira/browse/aurora-695 Repository: aurora Description --- - Update build files for the new syntax, which no longer requires 'pants(...)' around target names. - Remove no-longer-supported timeout from python_tests. Diffs - pants 4f9c351888afa1a779415730240093c3dee25dfb src/main/python/apache/aurora/admin/BUILD 7a100d1a4a74aae034082f34db051c9cc31f8540 src/main/python/apache/aurora/client/BUILD bf196bf86b36db0d72f8e096260c9a900f74d07c src/main/python/apache/aurora/client/api/BUILD 70ad38e34f14c6d54b71c8f4b2138f085658110e src/main/python/apache/aurora/client/bin/BUILD 43d747956df0611b0880f64df9955d5f5806901c src/main/python/apache/aurora/client/cli/BUILD ebe681a0d1735b7cc695dc3b7a14c4292d87ae32 src/main/python/apache/aurora/client/commands/BUILD cc16923909a7b26b0f3ac0b47bb37dafdbbf502e src/main/python/apache/aurora/client/hooks/BUILD 9471c4cba5175296030747301e246a65a39aa203 src/main/python/apache/aurora/common/BUILD b879b15127d6691b35880074fd0ceacd866a61ed src/main/python/apache/aurora/common/auth/BUILD 7e96cb2258711b2e2925d18ad9435fa986e86bca src/main/python/apache/aurora/config/BUILD 4f8fad80114ddabac8b25f70bba00119228ec675 src/main/python/apache/aurora/config/schema/BUILD 69d60aebd2a9aa353497406ae578a9997323b07e src/main/python/apache/aurora/executor/BUILD 1ad8f82cdce85cf228c53e088171918e36ed536d src/main/python/apache/aurora/executor/bin/BUILD aeb8aee6f50a0d89714626e933699c0a13b363d9 src/main/python/apache/aurora/executor/common/BUILD 335ebc4809096c5f128846cd846d33910a777968 src/main/python/apache/thermos/BUILD 0dc035f759dd9949997f0c979b3556a350cf8df7 src/main/python/apache/thermos/bin/BUILD 669f9930a3590184dc0f8b5c15c36168e715eb03 src/main/python/apache/thermos/common/BUILD 6015f9e9a23f71bf6dede1f4698fe63dbb4dcfaa src/main/python/apache/thermos/config/BUILD 0531f92ea569ffe36817b645a17fab7a712e5897 src/main/python/apache/thermos/core/BUILD 0d1d339d55ee6a569297614ac734661e5caf7ea4 src/main/python/apache/thermos/monitoring/BUILD 79da0d5cef9436d4a3d83075910decfc93e422a6 src/main/python/apache/thermos/observer/BUILD 49b844ffc1b1d5911fc28d14294d088c3d0b6e4b src/main/python/apache/thermos/observer/bin/BUILD 044ca66b18282daf17a4198ff369d954e14c9b6d src/main/python/apache/thermos/observer/http/BUILD 901ad9c61e4dd1c61f5fbf4467becb8c881a64ed src/main/python/apache/thermos/testing/BUILD dc328a63788381307576b5a43ecdc704bb764473 src/main/thrift/org/apache/aurora/gen/BUILD 947504ec1f9582496952b23e66d7f5f20a168ce7 src/test/python/BUILD f01efff2e4982a475221b5739dfe1e8fd1a41d92 src/test/python/apache/aurora/BUILD 6555b984a713ef786aef5688b206ae9d8017c48d src/test/python/apache/aurora/admin/BUILD 5e170d6c15a95e2511b69e18a255d7364c2e7a4d src/test/python/apache/aurora/client/BUILD 831a72d39b27ca2aca466a38914bbf40ff94 src/test/python/apache/aurora/client/api/BUILD b4f08c6192e6bf6b38665197e98db7235751ae86 src/test/python/apache/aurora/client/cli/BUILD e1f9ebf96774b8f5c75de8570c6ba87d953ab649 src/test/python/apache/aurora/client/commands/BUILD 17933dedfa08c9d12c369087bf801e7c35cdde9b src/test/python/apache/aurora/client/hooks/BUILD f7856a2d5dc7e5d1edc480f64d5db97d88c71b70 src/test/python/apache/aurora/common/BUILD e949ba8859d5567c62623bec9d5ba86a8463fbaa src/test/python/apache/aurora/config/BUILD 37bbd27e13a2a3589faff7285f04e3c44ca57eeb src/test/python/apache/aurora/executor/BUILD 4d43e256ad131223cc1ac36a406d42a979a8a2dd src/test/python/apache/aurora/executor/common/BUILD 7d8934046b56ac2c0c16440cfc571dc370767a14 src/test/python/apache/thermos/BUILD cb93a4622e33ef96855b89a7c138f42033368950 src/test/python/apache/thermos/bin/BUILD 4b59f3879298de9664f168150ea9029e013e7913 src/test/python/apache/thermos/common/BUILD 36fa6a69b5e77a645a65c52fef6ec9343bf541bc src/test/python/apache/thermos/config/BUILD 42445ceccba8dfe8296a22a174aca6123bdfdb52 src/test/python/apache/thermos/core/BUILD 8f5c626c2e89834dbb4938c3c69ef8c79558e12b src/test/python/apache/thermos/monitoring/BUILD ea4005b52be3185e553f7d23fb29b89f68befa50 Diff: https://reviews.apache.org/r/25543/diff/
Re: Review Request 25481: Adding JobUpdateRequest validation.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25481/#review53061 --- Aha, i thought you wanted this done down in JobUpdateController, so i put the validation in there. I'm happy to remove it from my diff. src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java https://reviews.apache.org/r/25481/#comment92450 I believe this will not surface nicely to the user, but will instead present as an internal error (based on LoggingInterceptor, which handles uncaught exceptions). - Bill Farner On Sept. 9, 2014, 7:46 p.m., Maxim Khutornenko wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25481/ --- (Updated Sept. 9, 2014, 7:46 p.m.) Review request for Aurora, Bill Farner and Zameer Manji. Bugs: AURORA-649 https://issues.apache.org/jira/browse/AURORA-649 Repository: aurora Description --- Adding JobUpdateRequest validation. Diffs - src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java a43e5d7748c22d60f56f03a8a3d52949faebeff2 src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java 0d51f7dc367081f72090736e36605bf363f3395e Diff: https://reviews.apache.org/r/25481/diff/ Testing --- gradle -Pq build Thanks, Maxim Khutornenko
Re: Review Request 25481: Adding JobUpdateRequest validation.
On Sept. 11, 2014, 5:54 p.m., Bill Farner wrote: src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java, line 1371 https://reviews.apache.org/r/25481/diff/1/?file=684118#file684118line1371 I believe this will not surface nicely to the user, but will instead present as an internal error (based on LoggingInterceptor, which handles uncaught exceptions). ERROR type is correctly consumed by the client. Here is an example from vagrant: ```INFO] Response from scheduler: ERROR (message: TaskQuery is missing.)``` - Maxim --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25481/#review53061 --- On Sept. 9, 2014, 7:46 p.m., Maxim Khutornenko wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25481/ --- (Updated Sept. 9, 2014, 7:46 p.m.) Review request for Aurora, Bill Farner and Zameer Manji. Bugs: AURORA-649 https://issues.apache.org/jira/browse/AURORA-649 Repository: aurora Description --- Adding JobUpdateRequest validation. Diffs - src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java a43e5d7748c22d60f56f03a8a3d52949faebeff2 src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java 0d51f7dc367081f72090736e36605bf363f3395e Diff: https://reviews.apache.org/r/25481/diff/ Testing --- gradle -Pq build Thanks, Maxim Khutornenko
Re: Review Request 25519: Adding get_scheduler admin command.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25519/#review53071 --- Ship it! Ship It! - Mark Chu-Carroll On Sept. 10, 2014, 6:43 p.m., Maxim Khutornenko wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25519/ --- (Updated Sept. 10, 2014, 6:43 p.m.) Review request for Aurora, Mark Chu-Carroll and Zameer Manji. Bugs: AURORA-692 https://issues.apache.org/jira/browse/AURORA-692 Repository: aurora Description --- Adding get_scheduler admin command. Diffs - src/main/python/apache/aurora/client/api/scheduler_client.py 0ba07611b1a367f7157b91a1d4b65b1af176 src/main/python/apache/aurora/client/commands/admin.py bc9a9eee9a187f2c895e70a093871f0b795931c4 src/test/python/apache/aurora/client/api/test_scheduler_client.py 630f662ad2ffb8d192299d98c612ad4892161081 src/test/python/apache/aurora/client/commands/test_admin.py 94e736fb80c3fd7f103437c24f33d7c4451a6969 src/test/python/apache/aurora/client/commands/util.py 21b8830df5a3eccc7d36067369fc16cc5fd9de2a src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh 989801cfcbd19109ac140b01cd3024d70c78c829 src/test/sh/org/apache/aurora/e2e/test_end_to_end_v2.sh 0965b5c8cb89eb36c6e15108c702c39dd68268be Diff: https://reviews.apache.org/r/25519/diff/ Testing --- ./pants src/test/python:all Thanks, Maxim Khutornenko
Re: Review Request 25481: Adding JobUpdateRequest validation.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25481/#review53073 --- src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java https://reviews.apache.org/r/25481/#comment92464 I'm a little unfamilar with JobUpdateRequest and this RPC but it seems we should update StartJobUpdateResult to have a message field that we can surface to the user? - Zameer Manji On Sept. 9, 2014, 12:46 p.m., Maxim Khutornenko wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25481/ --- (Updated Sept. 9, 2014, 12:46 p.m.) Review request for Aurora, Bill Farner and Zameer Manji. Bugs: AURORA-649 https://issues.apache.org/jira/browse/AURORA-649 Repository: aurora Description --- Adding JobUpdateRequest validation. Diffs - src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java a43e5d7748c22d60f56f03a8a3d52949faebeff2 src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java 0d51f7dc367081f72090736e36605bf363f3395e Diff: https://reviews.apache.org/r/25481/diff/ Testing --- gradle -Pq build Thanks, Maxim Khutornenko
Re: Review Request 25481: Adding JobUpdateRequest validation.
On Sept. 11, 2014, 6:13 p.m., Zameer Manji wrote: src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java, line 1371 https://reviews.apache.org/r/25481/diff/1/?file=684118#file684118line1371 I'm a little unfamilar with JobUpdateRequest and this RPC but it seems we should update StartJobUpdateResult to have a message field that we can surface to the user? Messages are already sent via thrift Response object. - Maxim --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25481/#review53073 --- On Sept. 9, 2014, 7:46 p.m., Maxim Khutornenko wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25481/ --- (Updated Sept. 9, 2014, 7:46 p.m.) Review request for Aurora, Bill Farner and Zameer Manji. Bugs: AURORA-649 https://issues.apache.org/jira/browse/AURORA-649 Repository: aurora Description --- Adding JobUpdateRequest validation. Diffs - src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java a43e5d7748c22d60f56f03a8a3d52949faebeff2 src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java 0d51f7dc367081f72090736e36605bf363f3395e Diff: https://reviews.apache.org/r/25481/diff/ Testing --- gradle -Pq build Thanks, Maxim Khutornenko
Re: Review Request 25481: Adding JobUpdateRequest validation.
On Sept. 11, 2014, 5:54 p.m., Bill Farner wrote: src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java, line 1371 https://reviews.apache.org/r/25481/diff/1/?file=684118#file684118line1371 I believe this will not surface nicely to the user, but will instead present as an internal error (based on LoggingInterceptor, which handles uncaught exceptions). Maxim Khutornenko wrote: ERROR type is correctly consumed by the client. Here is an example from vagrant: ```INFO] Response from scheduler: ERROR (message: TaskQuery is missing.)``` Right, but shouldn't we be returning `INVALID_REQUEST`? - Bill --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25481/#review53061 --- On Sept. 9, 2014, 7:46 p.m., Maxim Khutornenko wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25481/ --- (Updated Sept. 9, 2014, 7:46 p.m.) Review request for Aurora, Bill Farner and Zameer Manji. Bugs: AURORA-649 https://issues.apache.org/jira/browse/AURORA-649 Repository: aurora Description --- Adding JobUpdateRequest validation. Diffs - src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java a43e5d7748c22d60f56f03a8a3d52949faebeff2 src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java 0d51f7dc367081f72090736e36605bf363f3395e Diff: https://reviews.apache.org/r/25481/diff/ Testing --- gradle -Pq build Thanks, Maxim Khutornenko
Re: Review Request 25548: Apply GzipFilter to POSTs as well as GETs
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25548/#review53084
---
Ship it!
Ship It!
- Bill Farner
On Sept. 11, 2014, 6:20 p.m., Joshua Cohen wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25548/
---
(Updated Sept. 11, 2014, 6:20 p.m.)
Review request for Aurora, David McLaughlin and Bill Farner.
Bugs: AURORA-697
https://issues.apache.org/jira/browse/AURORA-697
Repository: aurora
Description
---
Apply GzipFilter to POSTs as well as GETs
Diffs
-
src/main/java/org/apache/aurora/scheduler/http/JettyServerModule.java
83ba0e49436034c8b6f9f736c60a726686096362
src/test/java/org/apache/aurora/scheduler/http/ServletFilterTest.java
2c31df6d0ac1c58e02b13d61ba9f511b781a20a7
Diff: https://reviews.apache.org/r/25548/diff/
Testing
---
./gradlew build -Pq
Also verified content-encoding against response from scheduler in vagrant
image:
$ curl -s -D - -o /dev/null -H Accept-Encoding: gzip -X POST -d @- \
http://192.168.33.7:8081/api EOF |tr '\r' '\n' |grep
Content-Encoding |awk '{print $2}' \
|tr '[:upper:]' '[:lower]'
[1,getJobSummary,1,0,{1:{str:vagrant}}]
EOF
gzip
Thanks,
Joshua Cohen
Re: Review Request 25529: Add a controller for job updates.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25529/#review53075 --- This matches the logic previously explained to me, but the diff is pretty large. Once my questions are answered I'll take another look before giving a ship it. build.gradle https://reviews.apache.org/r/25529/#comment92465 Is this needed? If so please break it out of this change. src/main/java/org/apache/aurora/scheduler/updater/InstanceActionHandler.java https://reviews.apache.org/r/25529/#comment92477 Would it be a bad idea to encapsulate this into a InstanceName class that contains the logic for constructing one from JobKey + instanceId and how to render it to a String? It seems like a bit of overkill but I dislike the + / in the code. src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java https://reviews.apache.org/r/25529/#comment92481 Just to be clear, currently-active updaters are also persisted in storage right? src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java https://reviews.apache.org/r/25529/#comment92483 Shouldn't this just be FAILED? src/main/java/org/apache/aurora/scheduler/updater/OneWayJobUpdaterFactory.java https://reviews.apache.org/r/25529/#comment92484 How are these errors going to be surfaced to the user? - Zameer Manji On Sept. 10, 2014, 9:53 p.m., Bill Farner wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25529/ --- (Updated Sept. 10, 2014, 9:53 p.m.) Review request for Aurora, Joshua Cohen, Kevin Sweeney, Maxim Khutornenko, and Zameer Manji. Bugs: AURORA-613 https://issues.apache.org/jira/browse/AURORA-613 Repository: aurora Description --- There's a lot going on here, i'm happy to break this up if you'd perfer, but the bulk of the change (JobUpdateControllerImpl+Test) would remain. The remaining required piece here is to record JobInstanceUpdateEvents when actions are taken. As you try to follow JobUpdateControllerImplTest, take some care to understand how FakeScheduledExecutor works. Ultimately, it accepts work added to a mock, and executes that work when FakeClock is advanced past the scheduled time. It may not be obvious at first, but be glad it's there - this kind of async test would be a nightmare without it. Diffs - build.gradle 3237f8dfa3e7d4249a388042dba840a939d513b3 src/main/java/org/apache/aurora/scheduler/async/RescheduleCalculator.java aaa3513ae99c70ffa51fec0a241d67fb815b6d3f src/main/java/org/apache/aurora/scheduler/base/Jobs.java 7b10cf876d4424fab06113aa3e2989a6bef4d346 src/main/java/org/apache/aurora/scheduler/base/Query.java d8572bb21a92025e7a51cf18d5bdf00fc1281078 src/main/java/org/apache/aurora/scheduler/base/Tasks.java 1be16374aeebaee59063aec982690ffa1fbdcf0f src/main/java/org/apache/aurora/scheduler/state/LockManagerImpl.java 8befecaf4f13c0b890b452bc7b9c0618725b8c41 src/main/java/org/apache/aurora/scheduler/storage/ForwardingStore.java b894a71348d2d71c077f35bb21a80ba88a6b4c42 src/main/java/org/apache/aurora/scheduler/storage/JobUpdateStore.java 599dbd8bb762d051b75aa1a1e4d6a4c90ca6eb87 src/main/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStore.java ec9b37ccaf03651e986aab9b4541f17ff0540008 src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java a43e5d7748c22d60f56f03a8a3d52949faebeff2 src/main/java/org/apache/aurora/scheduler/updater/InstanceActionHandler.java PRE-CREATION src/main/java/org/apache/aurora/scheduler/updater/InstanceUpdater.java d725bc356178f51464b4d8ea896f1bf76815e1b2 src/main/java/org/apache/aurora/scheduler/updater/JobUpdateController.java 39bdca02295706714cf720545ffc291f9042702a src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java 3f542ce3e45ec4561238736f4ce84b69f7e41219 src/main/java/org/apache/aurora/scheduler/updater/JobUpdateEventSubscriber.java 7be792f0bc9c5ec14c7001cb243a17d643f9607f src/main/java/org/apache/aurora/scheduler/updater/JobUpdateStateMachine.java 037602d6aabe6dda978cad008075c053e2c042eb src/main/java/org/apache/aurora/scheduler/updater/OneWayJobUpdater.java d6a1e4b2c916b1c4dbcc73fe79bd672fca9b3189 src/main/java/org/apache/aurora/scheduler/updater/OneWayJobUpdaterFactory.java 80baa7f9360cc8b2bf7910c26d119d443d6cbbf9 src/main/java/org/apache/aurora/scheduler/updater/UpdateConfigurationException.java PRE-CREATION src/main/java/org/apache/aurora/scheduler/updater/UpdaterModule.java 028cb079316ca48bb93a35913ae25ace78088fb4
Re: Review Request 25481: Adding JobUpdateRequest validation.
On Sept. 11, 2014, 5:54 p.m., Bill Farner wrote: src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java, line 1371 https://reviews.apache.org/r/25481/diff/1/?file=684118#file684118line1371 I believe this will not surface nicely to the user, but will instead present as an internal error (based on LoggingInterceptor, which handles uncaught exceptions). Maxim Khutornenko wrote: ERROR type is correctly consumed by the client. Here is an example from vagrant: ```INFO] Response from scheduler: ERROR (message: TaskQuery is missing.)``` Bill Farner wrote: Right, but shouldn't we be returning `INVALID_REQUEST`? Good point. Raised https://issues.apache.org/jira/browse/AURORA-701 to validate on the client instead. These should rather stay ERRORs to catch API-side violations. - Maxim --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25481/#review53061 --- On Sept. 9, 2014, 7:46 p.m., Maxim Khutornenko wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25481/ --- (Updated Sept. 9, 2014, 7:46 p.m.) Review request for Aurora, Bill Farner and Zameer Manji. Bugs: AURORA-649 https://issues.apache.org/jira/browse/AURORA-649 Repository: aurora Description --- Adding JobUpdateRequest validation. Diffs - src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java a43e5d7748c22d60f56f03a8a3d52949faebeff2 src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java 0d51f7dc367081f72090736e36605bf363f3395e Diff: https://reviews.apache.org/r/25481/diff/ Testing --- gradle -Pq build Thanks, Maxim Khutornenko
Review Request 25552: Show reason for PENDING state in Scheduler UI
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25552/ --- Review request for Aurora, Joshua Cohen and Maxim Khutornenko. Bugs: AURORA-495 https://issues.apache.org/jira/browse/AURORA-495 Repository: aurora Description --- Consumed the getPendingReason API call to add a message to any PENDING tasks. Diffs - src/main/resources/org/apache/aurora/scheduler/http/ui/js/services.js bfd360de45c75441743c8ba24a5c445f4146dba6 Diff: https://reviews.apache.org/r/25552/diff/ Testing --- ./gradlew jsHint Manual testing on local vagrant. Screenshots attached. File Attachments Screen Shot 2014-09-11 at 1.32.08 PM.png https://reviews.apache.org/media/uploaded/files/2014/09/11/e44e3909-ad9b-4c4c-a3e6-6b6b3aedaa3c__Screen_Shot_2014-09-11_at_1.32.08_PM.png Thanks, David McLaughlin
Re: Review Request 25548: Apply GzipFilter to POSTs as well as GETs
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25548/#review53105
---
Ship it!
Ship It!
- David McLaughlin
On Sept. 11, 2014, 6:20 p.m., Joshua Cohen wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25548/
---
(Updated Sept. 11, 2014, 6:20 p.m.)
Review request for Aurora, David McLaughlin and Bill Farner.
Bugs: AURORA-697
https://issues.apache.org/jira/browse/AURORA-697
Repository: aurora
Description
---
Apply GzipFilter to POSTs as well as GETs
Diffs
-
src/main/java/org/apache/aurora/scheduler/http/JettyServerModule.java
83ba0e49436034c8b6f9f736c60a726686096362
src/test/java/org/apache/aurora/scheduler/http/ServletFilterTest.java
2c31df6d0ac1c58e02b13d61ba9f511b781a20a7
Diff: https://reviews.apache.org/r/25548/diff/
Testing
---
./gradlew build -Pq
Also verified content-encoding against response from scheduler in vagrant
image:
$ curl -s -D - -o /dev/null -H Accept-Encoding: gzip -X POST -d @- \
http://192.168.33.7:8081/api EOF |tr '\r' '\n' |grep
Content-Encoding |awk '{print $2}' \
|tr '[:upper:]' '[:lower]'
[1,getJobSummary,1,0,{1:{str:vagrant}}]
EOF
gzip
Thanks,
Joshua Cohen
Re: Review Request 25548: Apply GzipFilter to POSTs as well as GETs
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25548/#review53109
---
Pushed to Apache master.
- David McLaughlin
On Sept. 11, 2014, 6:20 p.m., Joshua Cohen wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25548/
---
(Updated Sept. 11, 2014, 6:20 p.m.)
Review request for Aurora, David McLaughlin and Bill Farner.
Bugs: AURORA-697
https://issues.apache.org/jira/browse/AURORA-697
Repository: aurora
Description
---
Apply GzipFilter to POSTs as well as GETs
Diffs
-
src/main/java/org/apache/aurora/scheduler/http/JettyServerModule.java
83ba0e49436034c8b6f9f736c60a726686096362
src/test/java/org/apache/aurora/scheduler/http/ServletFilterTest.java
2c31df6d0ac1c58e02b13d61ba9f511b781a20a7
Diff: https://reviews.apache.org/r/25548/diff/
Testing
---
./gradlew build -Pq
Also verified content-encoding against response from scheduler in vagrant
image:
$ curl -s -D - -o /dev/null -H Accept-Encoding: gzip -X POST -d @- \
http://192.168.33.7:8081/api EOF |tr '\r' '\n' |grep
Content-Encoding |awk '{print $2}' \
|tr '[:upper:]' '[:lower]'
[1,getJobSummary,1,0,{1:{str:vagrant}}]
EOF
gzip
Thanks,
Joshua Cohen
Review Request 25556: Dropping synchronized from validateIfLocked()
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25556/ --- Review request for Aurora and Bill Farner. Repository: aurora Description --- Dropping synchronized from validateIfLocked() Diffs - src/main/java/org/apache/aurora/scheduler/state/LockManagerImpl.java 8befecaf4f13c0b890b452bc7b9c0618725b8c41 Diff: https://reviews.apache.org/r/25556/diff/ Testing --- right.. Thanks, Maxim Khutornenko
Re: Review Request 25556: Dropping synchronized from validateIfLocked()
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25556/ --- (Updated Sept. 11, 2014, 9:08 p.m.) Review request for Aurora and Bill Farner. Bugs: AURORA-702 https://issues.apache.org/jira/browse/AURORA-702 Repository: aurora Description --- Dropping synchronized from validateIfLocked() Diffs - src/main/java/org/apache/aurora/scheduler/state/LockManagerImpl.java 8befecaf4f13c0b890b452bc7b9c0618725b8c41 Diff: https://reviews.apache.org/r/25556/diff/ Testing --- right.. Thanks, Maxim Khutornenko
Re: Review Request 25391: Add information about log initialisation step.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25391/#review53110 --- Thanks Joseph! Pushed to master. - David McLaughlin On Sept. 5, 2014, 3:45 p.m., Joseph Glanville wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25391/ --- (Updated Sept. 5, 2014, 3:45 p.m.) Review request for Aurora, David McLaughlin and Bill Farner. Repository: aurora Description --- This tripped me up when doing a manual deployment without the help of the Vagrant scripts. I am fairly certain it also pops up on IRC fairly often, it would be good to document this here so newcomers don't get stuck on it. Diffs - docs/deploying-aurora-scheduler.md 25767fe Diff: https://reviews.apache.org/r/25391/diff/ Testing --- Thanks, Joseph Glanville
Re: Review Request 25552: Show reason for PENDING state in Scheduler UI
On Sept. 11, 2014, 8:45 p.m., Joshua Cohen wrote: src/main/resources/org/apache/aurora/scheduler/http/ui/js/services.js, line 90 https://reviews.apache.org/r/25552/diff/1/?file=686507#file686507line90 I think this could just be: _.indexBy(reasons, 'taskId') Good catch. Fixed. - David --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25552/#review53106 --- On Sept. 11, 2014, 9:55 p.m., David McLaughlin wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25552/ --- (Updated Sept. 11, 2014, 9:55 p.m.) Review request for Aurora, Joshua Cohen and Maxim Khutornenko. Bugs: AURORA-495 https://issues.apache.org/jira/browse/AURORA-495 Repository: aurora Description --- Consumed the getPendingReason API call to add a message to any PENDING tasks. Diffs - src/main/resources/org/apache/aurora/scheduler/http/ui/js/services.js bfd360de45c75441743c8ba24a5c445f4146dba6 Diff: https://reviews.apache.org/r/25552/diff/ Testing --- ./gradlew jsHint Manual testing on local vagrant. Screenshots attached. File Attachments Screen Shot 2014-09-11 at 1.32.08 PM.png https://reviews.apache.org/media/uploaded/files/2014/09/11/e44e3909-ad9b-4c4c-a3e6-6b6b3aedaa3c__Screen_Shot_2014-09-11_at_1.32.08_PM.png Thanks, David McLaughlin
Re: Review Request 25552: Show reason for PENDING state in Scheduler UI
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25552/ --- (Updated Sept. 11, 2014, 9:55 p.m.) Review request for Aurora, Joshua Cohen and Maxim Khutornenko. Changes --- Simplified underscore accessor. Bugs: AURORA-495 https://issues.apache.org/jira/browse/AURORA-495 Repository: aurora Description --- Consumed the getPendingReason API call to add a message to any PENDING tasks. Diffs (updated) - src/main/resources/org/apache/aurora/scheduler/http/ui/js/services.js bfd360de45c75441743c8ba24a5c445f4146dba6 Diff: https://reviews.apache.org/r/25552/diff/ Testing --- ./gradlew jsHint Manual testing on local vagrant. Screenshots attached. File Attachments Screen Shot 2014-09-11 at 1.32.08 PM.png https://reviews.apache.org/media/uploaded/files/2014/09/11/e44e3909-ad9b-4c4c-a3e6-6b6b3aedaa3c__Screen_Shot_2014-09-11_at_1.32.08_PM.png Thanks, David McLaughlin
Re: Review Request 25556: Dropping synchronized from validateIfLocked()
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25556/#review53121 --- Ship it! Please add a TODO for test coverage. Testing this for regression shouldn't be too hard, but let's get the fix in. - Bill Farner On Sept. 11, 2014, 9:08 p.m., Maxim Khutornenko wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25556/ --- (Updated Sept. 11, 2014, 9:08 p.m.) Review request for Aurora and Bill Farner. Bugs: AURORA-702 https://issues.apache.org/jira/browse/AURORA-702 Repository: aurora Description --- Dropping synchronized from validateIfLocked() Diffs - src/main/java/org/apache/aurora/scheduler/state/LockManagerImpl.java 8befecaf4f13c0b890b452bc7b9c0618725b8c41 Diff: https://reviews.apache.org/r/25556/diff/ Testing --- right.. Thanks, Maxim Khutornenko
Re: Review Request 25552: Show reason for PENDING state in Scheduler UI
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25552/#review53123 --- Ship it! Ship It! - Joshua Cohen On Sept. 11, 2014, 9:55 p.m., David McLaughlin wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25552/ --- (Updated Sept. 11, 2014, 9:55 p.m.) Review request for Aurora, Joshua Cohen and Maxim Khutornenko. Bugs: AURORA-495 https://issues.apache.org/jira/browse/AURORA-495 Repository: aurora Description --- Consumed the getPendingReason API call to add a message to any PENDING tasks. Diffs - src/main/resources/org/apache/aurora/scheduler/http/ui/js/services.js bfd360de45c75441743c8ba24a5c445f4146dba6 Diff: https://reviews.apache.org/r/25552/diff/ Testing --- ./gradlew jsHint Manual testing on local vagrant. Screenshots attached. File Attachments Screen Shot 2014-09-11 at 1.32.08 PM.png https://reviews.apache.org/media/uploaded/files/2014/09/11/e44e3909-ad9b-4c4c-a3e6-6b6b3aedaa3c__Screen_Shot_2014-09-11_at_1.32.08_PM.png Thanks, David McLaughlin
Re: Review Request 25529: Add a controller for job updates.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25529/#review53101 --- Some of these questions are probably non-sensical due to my limited understanding of the scheduler internals, apologies for that ;). src/main/java/org/apache/aurora/scheduler/updater/InstanceActionHandler.java https://reviews.apache.org/r/25529/#comment92509 Instead of embedding the logic into a switch here, would it make sense to make InstanceAction an actual class that encapsulates the logic/defines an interface around performing these actions? src/main/java/org/apache/aurora/scheduler/updater/InstanceActionHandler.java https://reviews.apache.org/r/25529/#comment92507 use instanceName here? src/main/java/org/apache/aurora/scheduler/updater/InstanceActionHandler.java https://reviews.apache.org/r/25529/#comment92508 Should we log (or assert?) here? Are there other actions that we're intentionally not processing, or is it an error if we get an action not covered? src/main/java/org/apache/aurora/scheduler/updater/InstanceUpdater.java https://reviews.apache.org/r/25529/#comment92513 Is there a ticket for this? src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java https://reviews.apache.org/r/25529/#comment92526 I don't know if the amount of work done by the underlying calls warrants it, but you could DRY this up a bit by having instance vars for `storeProvider.getJobUpdateStore()` (used here, and above) and `update.getSummary()` (used twice below and once above). If you wanted to go completely overboard, `update.getSummary().getJobKey()` is used above and below. src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java https://reviews.apache.org/r/25529/#comment92533 It seems like the logic of what states to transition to from a given state is somewhat spread out over the codebase (c.f. https://reviews.apache.org/r/25300/diff/#)? Is there any way we can centralize that? src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java https://reviews.apache.org/r/25529/#comment92537 Thoughts on changing the Function here to be `FunctionJobUpdateStatus, OptionalJobUpdateStatus` to clean up the null dance below? src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java https://reviews.apache.org/r/25529/#comment92542 Do we need guards in place in case the updaterStatus here is not ROLLING_BACK or ROLLING_FORWARD (or is it guaranteed to be one of the two? If so, is there danger in the addition of additional statuses in the future rendering that assumption invalid?) - Joshua Cohen On Sept. 11, 2014, 4:53 a.m., Bill Farner wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25529/ --- (Updated Sept. 11, 2014, 4:53 a.m.) Review request for Aurora, Joshua Cohen, Kevin Sweeney, Maxim Khutornenko, and Zameer Manji. Bugs: AURORA-613 https://issues.apache.org/jira/browse/AURORA-613 Repository: aurora Description --- There's a lot going on here, i'm happy to break this up if you'd perfer, but the bulk of the change (JobUpdateControllerImpl+Test) would remain. The remaining required piece here is to record JobInstanceUpdateEvents when actions are taken. As you try to follow JobUpdateControllerImplTest, take some care to understand how FakeScheduledExecutor works. Ultimately, it accepts work added to a mock, and executes that work when FakeClock is advanced past the scheduled time. It may not be obvious at first, but be glad it's there - this kind of async test would be a nightmare without it. Diffs - build.gradle 3237f8dfa3e7d4249a388042dba840a939d513b3 src/main/java/org/apache/aurora/scheduler/async/RescheduleCalculator.java aaa3513ae99c70ffa51fec0a241d67fb815b6d3f src/main/java/org/apache/aurora/scheduler/base/Jobs.java 7b10cf876d4424fab06113aa3e2989a6bef4d346 src/main/java/org/apache/aurora/scheduler/base/Query.java d8572bb21a92025e7a51cf18d5bdf00fc1281078 src/main/java/org/apache/aurora/scheduler/base/Tasks.java 1be16374aeebaee59063aec982690ffa1fbdcf0f src/main/java/org/apache/aurora/scheduler/state/LockManagerImpl.java 8befecaf4f13c0b890b452bc7b9c0618725b8c41 src/main/java/org/apache/aurora/scheduler/storage/ForwardingStore.java b894a71348d2d71c077f35bb21a80ba88a6b4c42 src/main/java/org/apache/aurora/scheduler/storage/JobUpdateStore.java 599dbd8bb762d051b75aa1a1e4d6a4c90ca6eb87 src/main/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStore.java ec9b37ccaf03651e986aab9b4541f17ff0540008
Re: Review Request 25556: Dropping synchronized from validateIfLocked()
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25556/#review53129 --- Feel free to merge in this test case if it's to your liking: https://github.com/wfarner/incubator-aurora/commit/a62f794c6052f6abfa53197dede7f6d21f80f4e4 - Bill Farner On Sept. 11, 2014, 9:08 p.m., Maxim Khutornenko wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25556/ --- (Updated Sept. 11, 2014, 9:08 p.m.) Review request for Aurora and Bill Farner. Bugs: AURORA-702 https://issues.apache.org/jira/browse/AURORA-702 Repository: aurora Description --- Dropping synchronized from validateIfLocked() Diffs - src/main/java/org/apache/aurora/scheduler/state/LockManagerImpl.java 8befecaf4f13c0b890b452bc7b9c0618725b8c41 Diff: https://reviews.apache.org/r/25556/diff/ Testing --- right.. Thanks, Maxim Khutornenko
Re: Review Request 25529: Add a controller for job updates.
On Sept. 11, 2014, 6:51 p.m., Zameer Manji wrote: build.gradle, line 262 https://reviews.apache.org/r/25529/diff/1/?file=685001#file685001line262 Is this needed? If so please break it out of this change. Context: this has code coverage run without adding -Pq Happy to pull this out for discussion in another review if you'd like. Basically, Maxim and i concluded that jacoco is a small overhead when compared to findbugs, and is a really important part of running unit tests. When trying to converge a unit test to 100% coverage, running the extra checks is a big burden. On Sept. 11, 2014, 6:51 p.m., Zameer Manji wrote: src/main/java/org/apache/aurora/scheduler/updater/InstanceActionHandler.java, line 109 https://reviews.apache.org/r/25529/diff/1/?file=685011#file685011line109 Would it be a bad idea to encapsulate this into a InstanceName class that contains the logic for constructing one from JobKey + instanceId and how to render it to a String? It seems like a bit of overkill but I dislike the + / in the code. Maybe a method rather than a class? The / is orthogonal to extracting a helper. I don't care about formatting - just that logs have necessary context. On Sept. 11, 2014, 6:51 p.m., Zameer Manji wrote: src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java, line 94 https://reviews.apache.org/r/25529/diff/1/?file=685014#file685014line94 Just to be clear, currently-active updaters are also persisted in storage right? Yes. On Sept. 11, 2014, 6:51 p.m., Zameer Manji wrote: src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java, line 357 https://reviews.apache.org/r/25529/diff/1/?file=685014#file685014line357 Shouldn't this just be FAILED? There's another static import for another FAILED. On Sept. 11, 2014, 6:51 p.m., Zameer Manji wrote: src/main/java/org/apache/aurora/scheduler/updater/OneWayJobUpdaterFactory.java, line 93 https://reviews.apache.org/r/25529/diff/1/?file=685018#file685018line93 How are these errors going to be surfaced to the user? Maxim and i stepped on toes here, so this validation moves up the stack with https://reviews.apache.org/r/25481/ - Bill --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25529/#review53075 --- On Sept. 11, 2014, 4:53 a.m., Bill Farner wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25529/ --- (Updated Sept. 11, 2014, 4:53 a.m.) Review request for Aurora, Joshua Cohen, Kevin Sweeney, Maxim Khutornenko, and Zameer Manji. Bugs: AURORA-613 https://issues.apache.org/jira/browse/AURORA-613 Repository: aurora Description --- There's a lot going on here, i'm happy to break this up if you'd perfer, but the bulk of the change (JobUpdateControllerImpl+Test) would remain. The remaining required piece here is to record JobInstanceUpdateEvents when actions are taken. As you try to follow JobUpdateControllerImplTest, take some care to understand how FakeScheduledExecutor works. Ultimately, it accepts work added to a mock, and executes that work when FakeClock is advanced past the scheduled time. It may not be obvious at first, but be glad it's there - this kind of async test would be a nightmare without it. Diffs - build.gradle 3237f8dfa3e7d4249a388042dba840a939d513b3 src/main/java/org/apache/aurora/scheduler/async/RescheduleCalculator.java aaa3513ae99c70ffa51fec0a241d67fb815b6d3f src/main/java/org/apache/aurora/scheduler/base/Jobs.java 7b10cf876d4424fab06113aa3e2989a6bef4d346 src/main/java/org/apache/aurora/scheduler/base/Query.java d8572bb21a92025e7a51cf18d5bdf00fc1281078 src/main/java/org/apache/aurora/scheduler/base/Tasks.java 1be16374aeebaee59063aec982690ffa1fbdcf0f src/main/java/org/apache/aurora/scheduler/state/LockManagerImpl.java 8befecaf4f13c0b890b452bc7b9c0618725b8c41 src/main/java/org/apache/aurora/scheduler/storage/ForwardingStore.java b894a71348d2d71c077f35bb21a80ba88a6b4c42 src/main/java/org/apache/aurora/scheduler/storage/JobUpdateStore.java 599dbd8bb762d051b75aa1a1e4d6a4c90ca6eb87 src/main/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStore.java ec9b37ccaf03651e986aab9b4541f17ff0540008 src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java a43e5d7748c22d60f56f03a8a3d52949faebeff2 src/main/java/org/apache/aurora/scheduler/updater/InstanceActionHandler.java PRE-CREATION src/main/java/org/apache/aurora/scheduler/updater/InstanceUpdater.java d725bc356178f51464b4d8ea896f1bf76815e1b2
Re: Review Request 25529: Add a controller for job updates.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25529/#review53051 --- src/main/java/org/apache/aurora/scheduler/base/Query.java https://reviews.apache.org/r/25529/#comment92433 typo src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java https://reviews.apache.org/r/25529/#comment92482 Consider discarding in favor of https://reviews.apache.org/r/25481/ src/main/java/org/apache/aurora/scheduler/updater/InstanceActionHandler.java https://reviews.apache.org/r/25529/#comment92439 Should it rather be named ADD_TASK...? It's used for both update (replace) and add new instance but since KILL is separate feels like ADD should be more appropriate here (i.e. it only inserts pending and does not do any killing). src/main/java/org/apache/aurora/scheduler/updater/InstanceActionHandler.java https://reviews.apache.org/r/25529/#comment92438 Replace with instanceName. src/main/java/org/apache/aurora/scheduler/updater/InstanceActionHandler.java https://reviews.apache.org/r/25529/#comment92488 Will quota checking come later or you'd rather see AURORA-686 fixed instead? src/main/java/org/apache/aurora/scheduler/updater/InstanceActionHandler.java https://reviews.apache.org/r/25529/#comment92440 This will throw unnecesserily if the instance is not in active state. I'd rather see an inactive instance get killed. src/main/java/org/apache/aurora/scheduler/updater/InstanceActionHandler.java https://reviews.apache.org/r/25529/#comment92463 This value is user-controlled and might be set too low for the kill to succeed. Would it rather make sense to fix it internally (as it is now on the client)? src/main/java/org/apache/aurora/scheduler/updater/JobUpdateController.java https://reviews.apache.org/r/25529/#comment92473 Missing docs. src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java https://reviews.apache.org/r/25529/#comment92480 typo src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java https://reviews.apache.org/r/25529/#comment92496 Why not just inline Functions.forMap(ImmutableMap.of(ABORTED, ABORTED)) instead? src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java https://reviews.apache.org/r/25529/#comment92495 Spent quite a bit of time reading this method. Would definitely benefit from refactoring. How about at least moving everything down from here to something like maybeEvaluateUpdater()? src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java https://reviews.apache.org/r/25529/#comment92527 Mind leaving a TODO here to create a getJobUpdate() storage API instead? src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java https://reviews.apache.org/r/25529/#comment92528 +1 src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java https://reviews.apache.org/r/25529/#comment92529 Is active() deliberate here? If so, perhaps rename the function to getActiveInstance to clearly state its purpose? src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java https://reviews.apache.org/r/25529/#comment92571 I a bit concerned about this circular dependency between evaluateUpdater and changeJobUpdateStatus but not sure what would be the best way to break it... Hopefully there is enough checking in place to prevent looping. src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java https://reviews.apache.org/r/25529/#comment92569 This seems unconditional. Where does rollbackOnFailure get evaluated? src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java https://reviews.apache.org/r/25529/#comment92570 Would it make sense to have a catch-all else block here to log in case actions is empty? src/main/java/org/apache/aurora/scheduler/updater/JobUpdateEventSubscriber.java https://reviews.apache.org/r/25529/#comment92497 Logging try...catch around similar to taskChangedState()? src/main/java/org/apache/aurora/scheduler/updater/JobUpdateEventSubscriber.java https://reviews.apache.org/r/25529/#comment92498 Same here. System resume is notoriously hard to troubleshoot and we probably don't want to let it out uncaught. src/test/java/org/apache/aurora/scheduler/updater/JobUpdaterIT.java https://reviews.apache.org/r/25529/#comment92575 s/time.s/times. - Maxim Khutornenko On Sept. 11, 2014, 4:53 a.m., Bill Farner wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25529/ --- (Updated Sept. 11, 2014, 4:53 a.m.) Review request for
Re: Review Request 25556: Dropping synchronized from validateIfLocked()
On Sept. 11, 2014, 10:45 p.m., Bill Farner wrote: Feel free to merge in this test case if it's to your liking: https://github.com/wfarner/incubator-aurora/commit/a62f794c6052f6abfa53197dede7f6d21f80f4e4 Big thanks for the patch! Updated. - Maxim --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25556/#review53129 --- On Sept. 11, 2014, 9:08 p.m., Maxim Khutornenko wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25556/ --- (Updated Sept. 11, 2014, 9:08 p.m.) Review request for Aurora and Bill Farner. Bugs: AURORA-702 https://issues.apache.org/jira/browse/AURORA-702 Repository: aurora Description --- Dropping synchronized from validateIfLocked() Diffs - src/main/java/org/apache/aurora/scheduler/state/LockManagerImpl.java 8befecaf4f13c0b890b452bc7b9c0618725b8c41 Diff: https://reviews.apache.org/r/25556/diff/ Testing --- right.. Thanks, Maxim Khutornenko
Re: Review Request 25556: Dropping synchronized from validateIfLocked()
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25556/ --- (Updated Sept. 12, 2014, 12:13 a.m.) Review request for Aurora and Bill Farner. Changes --- Unit test courtesy of wfarner. Bugs: AURORA-702 https://issues.apache.org/jira/browse/AURORA-702 Repository: aurora Description --- Dropping synchronized from validateIfLocked() Diffs (updated) - src/main/java/org/apache/aurora/scheduler/state/LockManagerImpl.java 8befecaf4f13c0b890b452bc7b9c0618725b8c41 src/test/java/org/apache/aurora/scheduler/state/LockManagerImplTest.java 791aa6fed7380999ea9257d92d16b69ed89dcea0 Diff: https://reviews.apache.org/r/25556/diff/ Testing --- right.. Thanks, Maxim Khutornenko
