Review Request 30389: Deny attempts to create a job update with a non-service job.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30389/ --- Review request for Aurora, Maxim Khutornenko and Zameer Manji. Bugs: AURORA-330 https://issues.apache.org/jira/browse/AURORA-330 Repository: aurora Description --- Deny attempts to create a job update with a non-service job. Diffs - src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java 8c19f3b08135eb5f3098591ebf9931b42a086318 src/main/python/apache/aurora/client/cli/jobs.py 7c5374417f8cca7400c7e92d014f706c0b2368fd src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java 03d1fba76c23570c2c4102a48daf5ce035ecaaa3 src/test/python/apache/aurora/client/cli/test_update.py c470ee64f11b5a1e4ce8cf1635c1acd2ec6e6e40 src/test/python/apache/aurora/client/cli/util.py 5b6207d45eba9ecc24cfd6dc5910677f9bc44372 Diff: https://reviews.apache.org/r/30389/diff/ Testing --- Thanks, Bill Farner
Re: Review Request 30389: Deny attempts to create a job update with a non-service job.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30389/#review70233 --- Ship it! Master (4f04a34) 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 Jan. 29, 2015, 5:13 p.m., Bill Farner wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30389/ --- (Updated Jan. 29, 2015, 5:13 p.m.) Review request for Aurora, Maxim Khutornenko and Zameer Manji. Bugs: AURORA-330 https://issues.apache.org/jira/browse/AURORA-330 Repository: aurora Description --- Deny attempts to create a job update with a non-service job. Diffs - src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java 8c19f3b08135eb5f3098591ebf9931b42a086318 src/main/python/apache/aurora/client/cli/jobs.py 7c5374417f8cca7400c7e92d014f706c0b2368fd src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java 03d1fba76c23570c2c4102a48daf5ce035ecaaa3 src/test/python/apache/aurora/client/cli/test_update.py c470ee64f11b5a1e4ce8cf1635c1acd2ec6e6e40 src/test/python/apache/aurora/client/cli/util.py 5b6207d45eba9ecc24cfd6dc5910677f9bc44372 Diff: https://reviews.apache.org/r/30389/diff/ Testing --- Thanks, Bill Farner
Re: Review Request 30389: Deny attempts to create a job update with a non-service job.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30389/#review70244 --- src/main/python/apache/aurora/client/cli/jobs.py https://reviews.apache.org/r/30389/#comment115321 Should we include the proper way to update a non-service job (kill/create) in this message? - Joshua Cohen On Jan. 29, 2015, 5:13 p.m., Bill Farner wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30389/ --- (Updated Jan. 29, 2015, 5:13 p.m.) Review request for Aurora, Maxim Khutornenko and Zameer Manji. Bugs: AURORA-330 https://issues.apache.org/jira/browse/AURORA-330 Repository: aurora Description --- Deny attempts to create a job update with a non-service job. Diffs - src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java 8c19f3b08135eb5f3098591ebf9931b42a086318 src/main/python/apache/aurora/client/cli/jobs.py 7c5374417f8cca7400c7e92d014f706c0b2368fd src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java 03d1fba76c23570c2c4102a48daf5ce035ecaaa3 src/test/python/apache/aurora/client/cli/test_update.py c470ee64f11b5a1e4ce8cf1635c1acd2ec6e6e40 src/test/python/apache/aurora/client/cli/util.py 5b6207d45eba9ecc24cfd6dc5910677f9bc44372 Diff: https://reviews.apache.org/r/30389/diff/ Testing --- Thanks, Bill Farner
Re: Review Request 30389: Deny attempts to create a job update with a non-service job.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30389/#review70248 --- src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java https://reviews.apache.org/r/30389/#comment115326 Why is this change necessary? src/test/python/apache/aurora/client/cli/test_update.py https://reviews.apache.org/r/30389/#comment115328 There has been some previous discussion that this style of test brittle and we should avoid it. Would you mind moving this test up to the TestJobUpdateCommand suite at the top of this file? This test could then be modified to have minimal patching. - Zameer Manji On Jan. 29, 2015, 9:13 a.m., Bill Farner wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30389/ --- (Updated Jan. 29, 2015, 9:13 a.m.) Review request for Aurora, Maxim Khutornenko and Zameer Manji. Bugs: AURORA-330 https://issues.apache.org/jira/browse/AURORA-330 Repository: aurora Description --- Deny attempts to create a job update with a non-service job. Diffs - src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java 8c19f3b08135eb5f3098591ebf9931b42a086318 src/main/python/apache/aurora/client/cli/jobs.py 7c5374417f8cca7400c7e92d014f706c0b2368fd src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java 03d1fba76c23570c2c4102a48daf5ce035ecaaa3 src/test/python/apache/aurora/client/cli/test_update.py c470ee64f11b5a1e4ce8cf1635c1acd2ec6e6e40 src/test/python/apache/aurora/client/cli/util.py 5b6207d45eba9ecc24cfd6dc5910677f9bc44372 Diff: https://reviews.apache.org/r/30389/diff/ Testing --- Thanks, Bill Farner
Re: Review Request 30389: Deny attempts to create a job update with a non-service job.
On Jan. 29, 2015, 6:49 p.m., Joshua Cohen wrote: src/main/python/apache/aurora/client/cli/jobs.py, line 741 https://reviews.apache.org/r/30389/diff/1/?file=839595#file839595line741 Should we include the proper way to update a non-service job (kill/create) in this message? Done. - Bill --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30389/#review70244 --- On Jan. 29, 2015, 5:13 p.m., Bill Farner wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30389/ --- (Updated Jan. 29, 2015, 5:13 p.m.) Review request for Aurora, Maxim Khutornenko and Zameer Manji. Bugs: AURORA-330 https://issues.apache.org/jira/browse/AURORA-330 Repository: aurora Description --- Deny attempts to create a job update with a non-service job. Diffs - src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java 8c19f3b08135eb5f3098591ebf9931b42a086318 src/main/python/apache/aurora/client/cli/jobs.py 7c5374417f8cca7400c7e92d014f706c0b2368fd src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java 03d1fba76c23570c2c4102a48daf5ce035ecaaa3 src/test/python/apache/aurora/client/cli/test_update.py c470ee64f11b5a1e4ce8cf1635c1acd2ec6e6e40 src/test/python/apache/aurora/client/cli/util.py 5b6207d45eba9ecc24cfd6dc5910677f9bc44372 Diff: https://reviews.apache.org/r/30389/diff/ Testing --- Thanks, Bill Farner
Re: Review Request 30389: Deny attempts to create a job update with a non-service job.
On Jan. 29, 2015, 7:15 p.m., Zameer Manji wrote: src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java, line 3116 https://reviews.apache.org/r/30389/diff/1/?file=839596#file839596line3116 Why is this change necessary? Thrift-generated code assumes collections are mutable, specifically in the `addTo*` methods. This is used in `ConfigurationManager`. On Jan. 29, 2015, 7:15 p.m., Zameer Manji wrote: src/test/python/apache/aurora/client/cli/test_update.py, line 464 https://reviews.apache.org/r/30389/diff/1/?file=839597#file839597line464 There has been some previous discussion that this style of test brittle and we should avoid it. Would you mind moving this test up to the TestJobUpdateCommand suite at the top of this file? This test could then be modified to have minimal patching. I had no idea there was a bifurcation of test approach in this file...wow. Moved. - Bill --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30389/#review70248 --- On Jan. 29, 2015, 5:13 p.m., Bill Farner wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30389/ --- (Updated Jan. 29, 2015, 5:13 p.m.) Review request for Aurora, Maxim Khutornenko and Zameer Manji. Bugs: AURORA-330 https://issues.apache.org/jira/browse/AURORA-330 Repository: aurora Description --- Deny attempts to create a job update with a non-service job. Diffs - src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java 8c19f3b08135eb5f3098591ebf9931b42a086318 src/main/python/apache/aurora/client/cli/jobs.py 7c5374417f8cca7400c7e92d014f706c0b2368fd src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java 03d1fba76c23570c2c4102a48daf5ce035ecaaa3 src/test/python/apache/aurora/client/cli/test_update.py c470ee64f11b5a1e4ce8cf1635c1acd2ec6e6e40 src/test/python/apache/aurora/client/cli/util.py 5b6207d45eba9ecc24cfd6dc5910677f9bc44372 Diff: https://reviews.apache.org/r/30389/diff/ Testing --- Thanks, Bill Farner
Re: Review Request 30389: Deny attempts to create a job update with a non-service job.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30389/#review70252 --- src/main/python/apache/aurora/client/cli/jobs.py https://reviews.apache.org/r/30389/#comment115331 This is techincally not true for the client updater. Cron jobs are supported despite the fact that we now have cron commands to do so. I suggest narrowing down this validation to ad-hoc jobs only. - Maxim Khutornenko On Jan. 29, 2015, 5:13 p.m., Bill Farner wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30389/ --- (Updated Jan. 29, 2015, 5:13 p.m.) Review request for Aurora, Maxim Khutornenko and Zameer Manji. Bugs: AURORA-330 https://issues.apache.org/jira/browse/AURORA-330 Repository: aurora Description --- Deny attempts to create a job update with a non-service job. Diffs - src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java 8c19f3b08135eb5f3098591ebf9931b42a086318 src/main/python/apache/aurora/client/cli/jobs.py 7c5374417f8cca7400c7e92d014f706c0b2368fd src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java 03d1fba76c23570c2c4102a48daf5ce035ecaaa3 src/test/python/apache/aurora/client/cli/test_update.py c470ee64f11b5a1e4ce8cf1635c1acd2ec6e6e40 src/test/python/apache/aurora/client/cli/util.py 5b6207d45eba9ecc24cfd6dc5910677f9bc44372 Diff: https://reviews.apache.org/r/30389/diff/ Testing --- Thanks, Bill Farner
Re: Review Request 30389: Deny attempts to create a job update with a non-service job.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30389/ --- (Updated Jan. 29, 2015, 7:31 p.m.) Review request for Aurora, Maxim Khutornenko and Zameer Manji. Bugs: AURORA-330 https://issues.apache.org/jira/browse/AURORA-330 Repository: aurora Description --- Deny attempts to create a job update with a non-service job. Diffs (updated) - src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java 8c19f3b08135eb5f3098591ebf9931b42a086318 src/main/python/apache/aurora/client/cli/jobs.py 7c5374417f8cca7400c7e92d014f706c0b2368fd src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java 03d1fba76c23570c2c4102a48daf5ce035ecaaa3 src/test/python/apache/aurora/client/cli/test_inspect.py ae04776fc32b2f09ce6303aeb893c60963896d60 src/test/python/apache/aurora/client/cli/test_update.py c470ee64f11b5a1e4ce8cf1635c1acd2ec6e6e40 src/test/python/apache/aurora/client/cli/util.py 5b6207d45eba9ecc24cfd6dc5910677f9bc44372 Diff: https://reviews.apache.org/r/30389/diff/ Testing --- Thanks, Bill Farner
Re: Review Request 30389: Deny attempts to create a job update with a non-service job.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30389/#review70255 --- Ship it! src/test/python/apache/aurora/client/cli/test_update.py https://reviews.apache.org/r/30389/#comment115334 This seems like a code smell. Shouldn't we be able to configure the fake_context to return the job_config instead of setting the method to a mock? This doesn't block my shipping of this patch but I would appreciate some investigation into this pattern. - Zameer Manji On Jan. 29, 2015, 11:31 a.m., Bill Farner wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30389/ --- (Updated Jan. 29, 2015, 11:31 a.m.) Review request for Aurora, Maxim Khutornenko and Zameer Manji. Bugs: AURORA-330 https://issues.apache.org/jira/browse/AURORA-330 Repository: aurora Description --- Deny attempts to create a job update with a non-service job. Diffs - src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java 8c19f3b08135eb5f3098591ebf9931b42a086318 src/main/python/apache/aurora/client/cli/jobs.py 7c5374417f8cca7400c7e92d014f706c0b2368fd src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java 03d1fba76c23570c2c4102a48daf5ce035ecaaa3 src/test/python/apache/aurora/client/cli/test_inspect.py ae04776fc32b2f09ce6303aeb893c60963896d60 src/test/python/apache/aurora/client/cli/test_update.py c470ee64f11b5a1e4ce8cf1635c1acd2ec6e6e40 src/test/python/apache/aurora/client/cli/util.py 5b6207d45eba9ecc24cfd6dc5910677f9bc44372 Diff: https://reviews.apache.org/r/30389/diff/ Testing --- Thanks, Bill Farner
Re: Review Request 28731: Implemented TaskScheduler benchmarks.
On Jan. 21, 2015, 10:19 a.m., Bill Farner wrote: Kevin - any chance you're available to take a pass at this soon? My attempts to free up a chunk of time for a thoughtful review have so far failed, and this is blocking other work of Maxim's. I'm afraid my review plate is full for this week, is anyone else able to step in? - Kevin --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28731/#review68950 --- On Jan. 21, 2015, 5:54 p.m., Maxim Khutornenko wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28731/ --- (Updated Jan. 21, 2015, 5:54 p.m.) Review request for Aurora, Kevin Sweeney and Bill Farner. Repository: aurora Description --- Added baseline benchmarks for a few static veto cases. Diffs - build.gradle f9f71a84493b782e9f6072e44e89a2c017cf2a09 src/jmh/java/org/apache/aurora/benchmark/Hosts.java PRE-CREATION src/jmh/java/org/apache/aurora/benchmark/Offers.java PRE-CREATION src/jmh/java/org/apache/aurora/benchmark/SchedulerBenchmark.java 5cecada93e4e04b689e826af49f691ed7e94ae49 src/jmh/java/org/apache/aurora/benchmark/SchedulingBenchmarks.java PRE-CREATION src/jmh/java/org/apache/aurora/benchmark/Tasks.java PRE-CREATION src/jmh/java/org/apache/aurora/benchmark/fakes/FakeDriver.java PRE-CREATION src/jmh/java/org/apache/aurora/benchmark/fakes/FakeRescheduleCalculator.java PRE-CREATION src/jmh/java/org/apache/aurora/benchmark/fakes/FakeStatsProvider.java PRE-CREATION src/main/java/org/apache/aurora/scheduler/async/TaskScheduler.java b6402ae42e3c7e4dca1c120fa6ef82d2d69e69d5 src/main/java/org/apache/aurora/scheduler/async/preemptor/CachedClusterState.java 03c2a8fde4a3fe5ac73f44da2cbe227995501c46 src/main/java/org/apache/aurora/scheduler/async/preemptor/ClusterState.java f7e157c890b5627411acd4bd5c2559ef4829147c src/main/java/org/apache/aurora/scheduler/async/preemptor/PreemptorImpl.java 0204d14b19ae412236f19ca274d81decb4eba12d Diff: https://reviews.apache.org/r/28731/diff/ Testing --- Sample run on a local box: ``` # VM invoker: /Library/Java/JavaVirtualMachines/jdk1.7.0_25.jdk/Contents/Home/jre/bin/java # VM options: -Dfile.encoding=UTF-8 -Duser.country=US -Duser.language=en -Duser.variant # Warmup: 10 iterations, 1 s each # Measurement: 100 iterations, 1 s each # Timeout: 10 min per iteration # Threads: 1 thread, will synchronize iterations # Benchmark mode: Average time, time/op # Benchmark: org.apache.aurora.benchmark.SchedulingBenchmarks.ConstraintMismatchsSchedulingBenchmark.runBenchmark # Run progress: 0.00% complete, ETA 00:05:30 # Fork: 1 of 1 # Warmup Iteration 1: 278284250.000 ns/op # Warmup Iteration 2: 70294312.500 ns/op # Warmup Iteration 3: 19293379.310 ns/op # Warmup Iteration 4: 11945387.097 ns/op # Warmup Iteration 5: 10725388.350 ns/op # Warmup Iteration 6: 13043282.353 ns/op # Warmup Iteration 7: 9233083.333 ns/op # Warmup Iteration 8: 9521051.724 ns/op # Warmup Iteration 9: 10750854.369 ns/op # Warmup Iteration 10: 7460243.243 ns/op Iteration 1: 7885364.286 ns/op Iteration 2: 7735139.860 ns/op Iteration 3: 7660208.333 ns/op Iteration 4: 7761204.225 ns/op Iteration 5: 7868907.143 ns/op Iteration 6: 7567404.110 ns/op Iteration 7: 7611000.000 ns/op Iteration 8: 7766154.930 ns/op Iteration 9: 7669344.828 ns/op Iteration 10: 7707783.217 ns/op Iteration 11: 7435651.007 ns/op Iteration 12: 7697631.944 ns/op Iteration 13: 7712531.469 ns/op Iteration 14: 7899407.143 ns/op Iteration 15: 7448472.973 ns/op Iteration 16: 7791521.127 ns/op Iteration 17: 7612213.793 ns/op Iteration 18: 7710867.133 ns/op Iteration 19: 7649296.552 ns/op Iteration 20: 7768309.859 ns/op Iteration 21: 7688666.667 ns/op Iteration 22: 7531557.823 ns/op Iteration 23: 7381193.333 ns/op Iteration 24: 7726006.993 ns/op Iteration 25: 7603358.621 ns/op Iteration 26: 7653631.944 ns/op Iteration 27: 7442275.168 ns/op Iteration 28: 7613186.207 ns/op Iteration 29: 7765823.944 ns/op Iteration 30: 7489687.075 ns/op Iteration 31: 7811443.662 ns/op Iteration 32: 8015007.246 ns/op Iteration 33: 8192392.593 ns/op Iteration 34: 8040335.766 ns/op Iteration 35: 7584212.329 ns/op Iteration 36: 8001934.783 ns/op Iteration 37: 9744815.789 ns/op Iteration 38: 11688284.211 ns/op Iteration 39: 8661406.250 ns/op Iteration 40: 7678413.793 ns/op Iteration 41: 8502223.077 ns/op Iteration 42: 7640820.690 ns/op Iteration 43: 7875624.113 ns/op Iteration 44: 7506809.524 ns/op Iteration 45: 8005431.655 ns/op Iteration 46: 8081664.234 ns/op Iteration
Re: Review Request 30389: Deny attempts to create a job update with a non-service job.
On Jan. 29, 2015, 7:36 p.m., Zameer Manji wrote: src/test/python/apache/aurora/client/cli/test_update.py, line 99 https://reviews.apache.org/r/30389/diff/1-2/?file=839597#file839597line99 This seems like a code smell. Shouldn't we be able to configure the fake_context to return the job_config instead of setting the method to a mock? This doesn't block my shipping of this patch but I would appreciate some investigation into this pattern. I choose to punt. This points out a more widespread issue with the overuse of `AuroraCommandContext`. I propose we aim to break down that class and turn it into a container class. Then, classes like this can inject mocks rather than constructing mocks for the purposes of wiring in other mocks. - Bill --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30389/#review70255 --- On Jan. 29, 2015, 7:31 p.m., Bill Farner wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30389/ --- (Updated Jan. 29, 2015, 7:31 p.m.) Review request for Aurora, Maxim Khutornenko and Zameer Manji. Bugs: AURORA-330 https://issues.apache.org/jira/browse/AURORA-330 Repository: aurora Description --- Deny attempts to create a job update with a non-service job. Diffs - src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java 8c19f3b08135eb5f3098591ebf9931b42a086318 src/main/python/apache/aurora/client/cli/jobs.py 7c5374417f8cca7400c7e92d014f706c0b2368fd src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java 03d1fba76c23570c2c4102a48daf5ce035ecaaa3 src/test/python/apache/aurora/client/cli/test_inspect.py ae04776fc32b2f09ce6303aeb893c60963896d60 src/test/python/apache/aurora/client/cli/test_update.py c470ee64f11b5a1e4ce8cf1635c1acd2ec6e6e40 src/test/python/apache/aurora/client/cli/util.py 5b6207d45eba9ecc24cfd6dc5910677f9bc44372 Diff: https://reviews.apache.org/r/30389/diff/ Testing --- Thanks, Bill Farner
Re: Review Request 30187: Remove support for cluster metadata in YAML format.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30187/#review70281 --- Brian - ping? - Bill Farner On Jan. 28, 2015, 8:26 p.m., Bill Farner wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30187/ --- (Updated Jan. 28, 2015, 8:26 p.m.) Review request for Aurora, Brian Wickman and Zameer Manji. Bugs: AURORA-1029 https://issues.apache.org/jira/browse/AURORA-1029 Repository: aurora Description --- Remove support for cluster metadata in YAML format. Diffs - src/main/python/apache/aurora/common/clusters.py e55aa774b4b868f696a7de51bb016f950871dd1e src/test/python/apache/aurora/common/BUILD 14165b96be99b8de418f4bb8def9f27eaf29e67d src/test/python/apache/aurora/common/test_clusters.py 45250e609cca1149dc296b2aaf645ff2f58f8288 Diff: https://reviews.apache.org/r/30187/diff/ Testing --- ./build-support/jenkins/build.sh test_end_to_end.sh is currently broken on master, i will address that and ensure it passes before committing this. Thanks, Bill Farner
Re: Review Request 30389: Deny attempts to create a job update with a non-service job.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30389/#review70274 --- Ship it! Ship It! - Joshua Cohen On Jan. 29, 2015, 7:31 p.m., Bill Farner wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30389/ --- (Updated Jan. 29, 2015, 7:31 p.m.) Review request for Aurora, Maxim Khutornenko and Zameer Manji. Bugs: AURORA-330 https://issues.apache.org/jira/browse/AURORA-330 Repository: aurora Description --- Deny attempts to create a job update with a non-service job. Diffs - src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java 8c19f3b08135eb5f3098591ebf9931b42a086318 src/main/python/apache/aurora/client/cli/jobs.py 7c5374417f8cca7400c7e92d014f706c0b2368fd src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java 03d1fba76c23570c2c4102a48daf5ce035ecaaa3 src/test/python/apache/aurora/client/cli/test_inspect.py ae04776fc32b2f09ce6303aeb893c60963896d60 src/test/python/apache/aurora/client/cli/test_update.py c470ee64f11b5a1e4ce8cf1635c1acd2ec6e6e40 src/test/python/apache/aurora/client/cli/util.py 5b6207d45eba9ecc24cfd6dc5910677f9bc44372 Diff: https://reviews.apache.org/r/30389/diff/ Testing --- Thanks, Bill Farner
Re: Review Request 28731: Implemented TaskScheduler benchmarks.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28731/ --- (Updated Jan. 29, 2015, 8:11 p.m.) Review request for Aurora, Bill Farner and Zameer Manji. Changes --- People -= kevints People += zmanji -wfarner Repository: aurora Description --- Added baseline benchmarks for a few static veto cases. Diffs - build.gradle f9f71a84493b782e9f6072e44e89a2c017cf2a09 src/jmh/java/org/apache/aurora/benchmark/Hosts.java PRE-CREATION src/jmh/java/org/apache/aurora/benchmark/Offers.java PRE-CREATION src/jmh/java/org/apache/aurora/benchmark/SchedulerBenchmark.java 5cecada93e4e04b689e826af49f691ed7e94ae49 src/jmh/java/org/apache/aurora/benchmark/SchedulingBenchmarks.java PRE-CREATION src/jmh/java/org/apache/aurora/benchmark/Tasks.java PRE-CREATION src/jmh/java/org/apache/aurora/benchmark/fakes/FakeDriver.java PRE-CREATION src/jmh/java/org/apache/aurora/benchmark/fakes/FakeRescheduleCalculator.java PRE-CREATION src/jmh/java/org/apache/aurora/benchmark/fakes/FakeStatsProvider.java PRE-CREATION src/main/java/org/apache/aurora/scheduler/async/TaskScheduler.java b6402ae42e3c7e4dca1c120fa6ef82d2d69e69d5 src/main/java/org/apache/aurora/scheduler/async/preemptor/CachedClusterState.java 03c2a8fde4a3fe5ac73f44da2cbe227995501c46 src/main/java/org/apache/aurora/scheduler/async/preemptor/ClusterState.java f7e157c890b5627411acd4bd5c2559ef4829147c src/main/java/org/apache/aurora/scheduler/async/preemptor/PreemptorImpl.java 0204d14b19ae412236f19ca274d81decb4eba12d Diff: https://reviews.apache.org/r/28731/diff/ Testing --- Sample run on a local box: ``` # VM invoker: /Library/Java/JavaVirtualMachines/jdk1.7.0_25.jdk/Contents/Home/jre/bin/java # VM options: -Dfile.encoding=UTF-8 -Duser.country=US -Duser.language=en -Duser.variant # Warmup: 10 iterations, 1 s each # Measurement: 100 iterations, 1 s each # Timeout: 10 min per iteration # Threads: 1 thread, will synchronize iterations # Benchmark mode: Average time, time/op # Benchmark: org.apache.aurora.benchmark.SchedulingBenchmarks.ConstraintMismatchsSchedulingBenchmark.runBenchmark # Run progress: 0.00% complete, ETA 00:05:30 # Fork: 1 of 1 # Warmup Iteration 1: 278284250.000 ns/op # Warmup Iteration 2: 70294312.500 ns/op # Warmup Iteration 3: 19293379.310 ns/op # Warmup Iteration 4: 11945387.097 ns/op # Warmup Iteration 5: 10725388.350 ns/op # Warmup Iteration 6: 13043282.353 ns/op # Warmup Iteration 7: 9233083.333 ns/op # Warmup Iteration 8: 9521051.724 ns/op # Warmup Iteration 9: 10750854.369 ns/op # Warmup Iteration 10: 7460243.243 ns/op Iteration 1: 7885364.286 ns/op Iteration 2: 7735139.860 ns/op Iteration 3: 7660208.333 ns/op Iteration 4: 7761204.225 ns/op Iteration 5: 7868907.143 ns/op Iteration 6: 7567404.110 ns/op Iteration 7: 7611000.000 ns/op Iteration 8: 7766154.930 ns/op Iteration 9: 7669344.828 ns/op Iteration 10: 7707783.217 ns/op Iteration 11: 7435651.007 ns/op Iteration 12: 7697631.944 ns/op Iteration 13: 7712531.469 ns/op Iteration 14: 7899407.143 ns/op Iteration 15: 7448472.973 ns/op Iteration 16: 7791521.127 ns/op Iteration 17: 7612213.793 ns/op Iteration 18: 7710867.133 ns/op Iteration 19: 7649296.552 ns/op Iteration 20: 7768309.859 ns/op Iteration 21: 7688666.667 ns/op Iteration 22: 7531557.823 ns/op Iteration 23: 7381193.333 ns/op Iteration 24: 7726006.993 ns/op Iteration 25: 7603358.621 ns/op Iteration 26: 7653631.944 ns/op Iteration 27: 7442275.168 ns/op Iteration 28: 7613186.207 ns/op Iteration 29: 7765823.944 ns/op Iteration 30: 7489687.075 ns/op Iteration 31: 7811443.662 ns/op Iteration 32: 8015007.246 ns/op Iteration 33: 8192392.593 ns/op Iteration 34: 8040335.766 ns/op Iteration 35: 7584212.329 ns/op Iteration 36: 8001934.783 ns/op Iteration 37: 9744815.789 ns/op Iteration 38: 11688284.211 ns/op Iteration 39: 8661406.250 ns/op Iteration 40: 7678413.793 ns/op Iteration 41: 8502223.077 ns/op Iteration 42: 7640820.690 ns/op Iteration 43: 7875624.113 ns/op Iteration 44: 7506809.524 ns/op Iteration 45: 8005431.655 ns/op Iteration 46: 8081664.234 ns/op Iteration 47: 7579438.356 ns/op Iteration 48: 7993405.797 ns/op Iteration 49: 7571958.904 ns/op Iteration 50: 8116463.235 ns/op Iteration 51: 7941330.935 ns/op Iteration 52: 7687145.833 ns/op Iteration 53: 8082554.745 ns/op Iteration 54: 7597889.655 ns/op Iteration 55: 7299907.285 ns/op Iteration 56: 7992789.855 ns/op Iteration 57: 7648268.966 ns/op Iteration 58: 7570863.014 ns/op Iteration 59: 7885078.571 ns/op Iteration 60: 7647158.621 ns/op Iteration 61: 7830858.156 ns/op Iteration 62: 7773690.141 ns/op Iteration 63: 7905850.000 ns/op Iteration 64: 7653800.000 ns/op Iteration 65: 7408248.322 ns/op Iteration 66: 7961352.518 ns/op Iteration 67: 7879785.714 ns/op Iteration 68:
Re: Review Request 30389: Deny attempts to create a job update with a non-service job.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30389/#review70269 --- Master (4f04a34) is red with this patch. ./build-support/jenkins/build.sh SUCCESS: /home/jenkins/jenkins-slave/workspace/AuroraBot/src/test/python/apache/aurora/client/cli/util.py Everything Looks Good! SUCCESS: /home/jenkins/jenkins-slave/workspace/AuroraBot/src/test/python/apache/aurora/admin/__init__.py Everything Looks Good! SUCCESS: /home/jenkins/jenkins-slave/workspace/AuroraBot/src/test/python/apache/aurora/admin/test_host_maintenance.py Everything Looks Good! SUCCESS: /home/jenkins/jenkins-slave/workspace/AuroraBot/src/test/python/apache/aurora/admin/test_admin_util.py Everything Looks Good! SUCCESS: /home/jenkins/jenkins-slave/workspace/AuroraBot/src/test/python/apache/aurora/admin/test_maintenance.py Everything Looks Good! SUCCESS: /home/jenkins/jenkins-slave/workspace/AuroraBot/src/test/python/apache/aurora/admin/test_admin.py Everything Looks Good! SUCCESS: /home/jenkins/jenkins-slave/workspace/AuroraBot/src/test/python/apache/aurora/admin/test_admin_sla.py Everything Looks Good! SUCCESS: /home/jenkins/jenkins-slave/workspace/AuroraBot/src/test/python/apache/aurora/admin/util.py Everything Looks Good! SUCCESS: /home/jenkins/jenkins-slave/workspace/AuroraBot/src/test/python/apache/aurora/common/test_transport.py Everything Looks Good! SUCCESS: /home/jenkins/jenkins-slave/workspace/AuroraBot/src/test/python/apache/aurora/common/test_cluster_option.py Everything Looks Good! SUCCESS: /home/jenkins/jenkins-slave/workspace/AuroraBot/src/test/python/apache/aurora/common/test_cluster.py Everything Looks Good! SUCCESS: /home/jenkins/jenkins-slave/workspace/AuroraBot/src/test/python/apache/aurora/common/test_clusters.py Everything Looks Good! SUCCESS: /home/jenkins/jenkins-slave/workspace/AuroraBot/src/test/python/apache/aurora/common/test_pex_version.py Everything Looks Good! SUCCESS: /home/jenkins/jenkins-slave/workspace/AuroraBot/src/test/python/apache/aurora/common/test_shellify.py Everything Looks Good! SUCCESS: /home/jenkins/jenkins-slave/workspace/AuroraBot/src/test/python/apache/aurora/common/test_aurora_job_key.py Everything Looks Good! SUCCESS: /home/jenkins/jenkins-slave/workspace/AuroraBot/src/test/python/apache/aurora/common/test_http_signaler.py Everything Looks Good! SUCCESS: /home/jenkins/jenkins-slave/workspace/AuroraBot/src/test/python/apache/aurora/config/test_constraint_parsing.py Everything Looks Good! SUCCESS: /home/jenkins/jenkins-slave/workspace/AuroraBot/src/test/python/apache/aurora/config/test_thrift.py Everything Looks Good! SUCCESS: /home/jenkins/jenkins-slave/workspace/AuroraBot/src/test/python/apache/aurora/config/test_loader.py Everything Looks Good! SUCCESS: /home/jenkins/jenkins-slave/workspace/AuroraBot/src/test/python/apache/aurora/config/test_base.py Everything Looks Good! SUCCESS: /home/jenkins/jenkins-slave/workspace/AuroraBot/src/test/python/apache/aurora/executor/test_status_manager.py Everything Looks Good! SUCCESS: /home/jenkins/jenkins-slave/workspace/AuroraBot/src/test/python/apache/aurora/executor/test_executor_vars.py Everything Looks Good! SUCCESS: /home/jenkins/jenkins-slave/workspace/AuroraBot/src/test/python/apache/aurora/executor/test_executor_base.py Everything Looks Good! SUCCESS: /home/jenkins/jenkins-slave/workspace/AuroraBot/src/test/python/apache/aurora/executor/test_thermos_executor.py Everything Looks Good! SUCCESS: /home/jenkins/jenkins-slave/workspace/AuroraBot/src/test/python/apache/aurora/executor/test_gc_executor.py Everything Looks Good! SUCCESS: /home/jenkins/jenkins-slave/workspace/AuroraBot/src/test/python/apache/aurora/executor/test_thermos_task_runner.py Everything Looks Good! SUCCESS: /home/jenkins/jenkins-slave/workspace/AuroraBot/src/test/python/apache/aurora/executor/test_executor_detector.py Everything Looks Good! SUCCESS: /home/jenkins/jenkins-slave/workspace/AuroraBot/src/test/python/apache/aurora/executor/common/test_announcer.py Everything Looks Good! SUCCESS: /home/jenkins/jenkins-slave/workspace/AuroraBot/src/test/python/apache/aurora/executor/common/test_kill_manager.py Everything Looks Good! SUCCESS: /home/jenkins/jenkins-slave/workspace/AuroraBot/src/test/python/apache/aurora/executor/common/__init__.py Everything Looks Good! SUCCESS: /home/jenkins/jenkins-slave/workspace/AuroraBot/src/test/python/apache/aurora/executor/common/test_executor_timeout.py Everything Looks Good! SUCCESS: /home/jenkins/jenkins-slave/workspace/AuroraBot/src/test/python/apache/aurora/executor/common/fixtures.py Everything Looks Good! SUCCESS: /home/jenkins/jenkins-slave/workspace/AuroraBot/src/test/python/apache/aurora/executor/common/test_health_checker.py Everything Looks Good! SUCCESS:
Re: Review Request 30389: Deny attempts to create a job update with a non-service job.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30389/ --- (Updated Jan. 29, 2015, 9:26 p.m.) Review request for Aurora, Maxim Khutornenko and Zameer Manji. Bugs: AURORA-330 https://issues.apache.org/jira/browse/AURORA-330 Repository: aurora Description --- Deny attempts to create a job update with a non-service job. Diffs (updated) - src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java 8c19f3b08135eb5f3098591ebf9931b42a086318 src/main/python/apache/aurora/client/cli/jobs.py 7c5374417f8cca7400c7e92d014f706c0b2368fd src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java 03d1fba76c23570c2c4102a48daf5ce035ecaaa3 src/test/python/apache/aurora/client/cli/test_inspect.py ae04776fc32b2f09ce6303aeb893c60963896d60 src/test/python/apache/aurora/client/cli/test_update.py c470ee64f11b5a1e4ce8cf1635c1acd2ec6e6e40 src/test/python/apache/aurora/client/cli/util.py 5b6207d45eba9ecc24cfd6dc5910677f9bc44372 Diff: https://reviews.apache.org/r/30389/diff/ Testing --- Thanks, Bill Farner
Re: Review Request 30225: Modifying update controller to support heartbeats.
On Jan. 28, 2015, 7:41 p.m., David McLaughlin wrote: src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java, lines 259-263 https://reviews.apache.org/r/30225/diff/1/?file=832014#file832014line259 I am unsure why this is being called inside pulse. Once pulse is activated, only the absence of a pulse can modify the update, right? We don't resume a paused update by receiving a pulse. So surely the last pulse time would be checked externally to the method that performs the pulse? If we can remove this, you can get rid of the write lock completely here, because all you need are strongly consistent reads (which we have) to accurately update the cooridinatedUpdateStates map correctly. Maxim Khutornenko wrote: An update blocked (not PAUSED) due to a missed pulse can be unblocked by a new pulse. This covers a few important design desisions: - An update can be created blocked by default awaiting for the first pulse to start its progress; - An occasional network partition/delay will not require an explicit external service operation to resume; - A scheduler restart is treated the same as initial update creation - an update is rehydrated and waits for a pulse to resume; More details and scenarios here: https://github.com/maxim111333/incubator-aurora/blob/hb_doc/docs/update-heartbeat.md David McLaughlin wrote: How do we show to the user (via client output or UI) that the update is currently blocked? Maxim Khutornenko wrote: One possible way is described here: https://issues.apache.org/jira/browse/AURORA-1049 David McLaughlin wrote: I don't think this is sufficient. We'd need auditing to explain to users why an update was paused (blocked) for a given time, not just the current status. Maxim Khutornenko wrote: That would require persistance and changing the actual status of the job. I'd rather not introduce a new state that would only be applicable to specific update configurations. The more important here is the visibility into the internal transient state to troubleshoot a coordinated job unable to make progress. David McLaughlin wrote: I don't follow the line of reasoning that 100% of updates have to use a feature for us to have a state to reflect it. I think in terms of simplicity and consistency, it makes way more sense to have an explicit UPDATE_WAITING_FOR_PULSE state that these updates are initialised in and moved to when blocked. A pulse could then have one valid state transition that would require a write: UPDATE_WAITING_FOR_PULSE - ROLLING_FORWARD. The drawbacks of this compared to this approach I think would be performance in the case of the external monitoring service flapping, or in the case of a scheduler failover. To combat this, we could also add some kind of initial grace period by setting a base timestamp at the point of leader election. This timestamp could be used in the should this be blocked? calculation when no previous pulse exists in the on-heap data structure. Maxim Khutornenko wrote: The consensus we reached on the dev list [1] is to implement the first version of the feature without any persistence at all. We agreed to look into reacher status reporting later as/if needed. IMO, adding a new state is a more involved solution. We would need to revisit the update state graph, figure out and validate the new rules for user pause/resume actions wrt the new state and pulse. Not impossible but hardly a blocking requirement either. [1] - http://mail-archives.apache.org/mod_mbox/incubator-aurora-dev/201410.mbox/browser David McLaughlin wrote: On that email thread, my last comment is to bring up this exact point :) I'm -1 on proceeding without auditing for blocked state. But if the majority disagrees, then that's fine and I'm fine for this to proceed with another ship it. Bill Farner wrote: David - I do think you raise a valid point, and we at least need to determine whether this is a reasonable tradeoff. Can you start a thread on dev@ to try to reach consensus? Done. - David --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30225/#review70058 --- On Jan. 23, 2015, 8:37 p.m., Maxim Khutornenko wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30225/ --- (Updated Jan. 23, 2015, 8:37 p.m.) Review request for Aurora, David McLaughlin, Joshua Cohen, and Bill Farner. Bugs: AURORA-1010 https://issues.apache.org/jira/browse/AURORA-1010 Repository: aurora Description
Review Request 30433: Prevent multiple active job updates from being inserted into storage.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30433/ --- Review request for Aurora and Maxim Khutornenko. Bugs: AURORA-1023 https://issues.apache.org/jira/browse/AURORA-1023 Repository: aurora Description --- This adds a guard before an update is saved to make sure we don't already have an active update for the given job. This was an implied invariant due to the relationship between the locks table and job updates table, but that relationship can be compromised by outside user action. Diffs - src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java a992938d4e12b20f81608be6bbdc24c0a211c3fd src/test/java/org/apache/aurora/scheduler/updater/JobUpdaterIT.java 4c827b183a87b4d97774edbfaa960bd1c3de72a5 Diff: https://reviews.apache.org/r/30433/diff/ Testing --- Thanks, Bill Farner
Re: Review Request 30433: Prevent multiple active job updates from being inserted into storage.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30433/#review70316 --- Ship it! src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java https://reviews.apache.org/r/30433/#comment115419 Suggest adding an explanation of what active update means, e.g.: An update in one of the following states is considered active + ACTIVE_JOB_UPDATE_STATES. Otherwise, it can be unclear why PAUSED update prevents from creating a new one. - Maxim Khutornenko On Jan. 30, 2015, 12:21 a.m., Bill Farner wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30433/ --- (Updated Jan. 30, 2015, 12:21 a.m.) Review request for Aurora and Maxim Khutornenko. Bugs: AURORA-1023 https://issues.apache.org/jira/browse/AURORA-1023 Repository: aurora Description --- This adds a guard before an update is saved to make sure we don't already have an active update for the given job. This was an implied invariant due to the relationship between the locks table and job updates table, but that relationship can be compromised by outside user action. Diffs - src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java a992938d4e12b20f81608be6bbdc24c0a211c3fd src/test/java/org/apache/aurora/scheduler/updater/JobUpdaterIT.java 4c827b183a87b4d97774edbfaa960bd1c3de72a5 Diff: https://reviews.apache.org/r/30433/diff/ Testing --- Thanks, Bill Farner
Re: Review Request 30446: Fix compile errors under Java 8.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30446/#review70332 --- Ship it! Master (4f04a34) 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 Jan. 30, 2015, 7:04 a.m., Joshua Cohen wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30446/ --- (Updated Jan. 30, 2015, 7:04 a.m.) Review request for Aurora, Kevin Sweeney and Bill Farner. Repository: aurora Description --- Fix compile errors under Java 8. I think this is just caused by the Java 8 compiler having better type inference than Java 7? I'm not in love with this fix, but the alternative is to declare these methods as throwing `Exception` which propagates out pretty widely. Happy to go that route if folks prefer, but figured I'd start with the more tactical fix. Diffs - src/test/java/org/apache/aurora/scheduler/storage/testing/StorageTestUtil.java d492e176e73cbd2b3c696fc488010db16106b36a Diff: https://reviews.apache.org/r/30446/diff/ Testing --- ./gradlew build -Pq Thanks, Joshua Cohen
Review Request 30446: Fix compile errors under Java 8.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30446/ --- Review request for Aurora, Kevin Sweeney and Bill Farner. Repository: aurora Description --- Fix compile errors under Java 8. I think this is just caused by the Java 8 compiler having better type inference than Java 7? I'm not in love with this fix, but the alternative is to declare these methods as throwing `Exception` which propagates out pretty widely. Happy to go that route if folks prefer, but figured I'd start with the more tactical fix. Diffs - src/test/java/org/apache/aurora/scheduler/storage/testing/StorageTestUtil.java d492e176e73cbd2b3c696fc488010db16106b36a Diff: https://reviews.apache.org/r/30446/diff/ Testing --- ./gradlew build -Pq Thanks, Joshua Cohen
