Re: Review Request 36436: Prevent job updates from allowing unbounded instance events
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36436/#review91749 --- src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java (line 169) https://reviews.apache.org/r/36436/#comment145378 For better readability, how about `max_update_instance_failures`? src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java (line 171) https://reviews.apache.org/r/36436/#comment145377 s/scheduler datastore/storage/ src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java (line 173) https://reviews.apache.org/r/36436/#comment145375 Unfortunately, this will not work as you expect and derive from the value of `-max_tasks_per_job`. This will always capture the default value (4000) since it is evaluated before the arg is populated. In fact, this would cause a failure if `-max_tasks_per_job` was set on the command line, since args prevent setting a value after it has been read. The best way i can see to accomplish this while maintaining the relationship is to extract constants: ``` private static final int DEFAULT_MAX_TASKS_PER_JOB = 4000; private static final int DEFAULT_MAX_INSTANCE_UPDATE_EVENT_FAILURES = DEFAULT_MAX_TASKS_PER_JOB * 5; ``` There isn't a clean way to have this multiplier effect that Just Works, so i suggest you just set it to 2 and leave a comment near `MAX_TASKS_PER_JOB` suggesting that this arg be changed if the default is changed there. src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java (line 1123) https://reviews.apache.org/r/36436/#comment145376 This first part can be ommitted, since it's effectively covered by `MAX_TASKS_PER_JOB`. - Bill Farner On July 15, 2015, 12:30 a.m., Joe Smith wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36436/ --- (Updated July 15, 2015, 12:30 a.m.) Review request for Aurora and Bill Farner. Bugs: AURORA-1096 https://issues.apache.org/jira/browse/AURORA-1096 Repository: aurora Description --- Prevent job updates from allowing unbounded instance events Diffs - src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java dc0cd2d71d11b8157154f7b63a14f0282dee09f1 src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java d28baba7618ebb194a61455971786aef46abd8eb Diff: https://reviews.apache.org/r/36436/diff/ Testing --- `./gradlew build -Pq` Thanks, Joe Smith
Re: Review Request 36436: Prevent job updates from allowing unbounded instance events
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36436/ --- (Updated July 15, 2015, 11:08 a.m.) Review request for Aurora and Bill Farner. Changes --- Bill's fixes Bugs: AURORA-1096 https://issues.apache.org/jira/browse/AURORA-1096 Repository: aurora Description --- Prevent job updates from allowing unbounded instance events Diffs (updated) - src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java dc0cd2d71d11b8157154f7b63a14f0282dee09f1 src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java d28baba7618ebb194a61455971786aef46abd8eb Diff: https://reviews.apache.org/r/36436/diff/ Testing --- `./gradlew build -Pq` Thanks, Joe Smith
Re: Review Request 36436: Prevent job updates from allowing unbounded instance events
On July 15, 2015, 9:50 a.m., Bill Farner wrote: src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java, line 169 https://reviews.apache.org/r/36436/diff/3/?file=1011900#file1011900line169 For better readability, how about `max_update_instance_failures`? Done. On July 15, 2015, 9:50 a.m., Bill Farner wrote: src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java, line 171 https://reviews.apache.org/r/36436/diff/3/?file=1011900#file1011900line171 s/scheduler datastore/storage/ Done. On July 15, 2015, 9:50 a.m., Bill Farner wrote: src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java, line 173 https://reviews.apache.org/r/36436/diff/3/?file=1011900#file1011900line173 Unfortunately, this will not work as you expect and derive from the value of `-max_tasks_per_job`. This will always capture the default value (4000) since it is evaluated before the arg is populated. In fact, this would cause a failure if `-max_tasks_per_job` was set on the command line, since args prevent setting a value after it has been read. The best way i can see to accomplish this while maintaining the relationship is to extract constants: ``` private static final int DEFAULT_MAX_TASKS_PER_JOB = 4000; private static final int DEFAULT_MAX_INSTANCE_UPDATE_EVENT_FAILURES = DEFAULT_MAX_TASKS_PER_JOB * 5; ``` There isn't a clean way to have this multiplier effect that Just Works, so i suggest you just set it to 2 and leave a comment near `MAX_TASKS_PER_JOB` suggesting that this arg be changed if the default is changed there. Aha, good to know. Thanks for catching that! Let me know if you think that comment is sufficient. On July 15, 2015, 9:50 a.m., Bill Farner wrote: src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java, line 1123 https://reviews.apache.org/r/36436/diff/3/?file=1011900#file1011900line1123 This first part can be ommitted, since it's effectively covered by `MAX_TASKS_PER_JOB`. Done. - Joe --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36436/#review91749 --- On July 14, 2015, 5:30 p.m., Joe Smith wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36436/ --- (Updated July 14, 2015, 5:30 p.m.) Review request for Aurora and Bill Farner. Bugs: AURORA-1096 https://issues.apache.org/jira/browse/AURORA-1096 Repository: aurora Description --- Prevent job updates from allowing unbounded instance events Diffs - src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java dc0cd2d71d11b8157154f7b63a14f0282dee09f1 src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java d28baba7618ebb194a61455971786aef46abd8eb Diff: https://reviews.apache.org/r/36436/diff/ Testing --- `./gradlew build -Pq` Thanks, Joe Smith
Re: Review Request 36436: Prevent job updates from allowing unbounded instance events
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36436/#review91786 --- Ship it! Master (d9dac92) 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 July 15, 2015, 6:08 p.m., Joe Smith wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36436/ --- (Updated July 15, 2015, 6:08 p.m.) Review request for Aurora and Bill Farner. Bugs: AURORA-1096 https://issues.apache.org/jira/browse/AURORA-1096 Repository: aurora Description --- Prevent job updates from allowing unbounded instance events Diffs - src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java dc0cd2d71d11b8157154f7b63a14f0282dee09f1 src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java d28baba7618ebb194a61455971786aef46abd8eb Diff: https://reviews.apache.org/r/36436/diff/ Testing --- `./gradlew build -Pq` Thanks, Joe Smith
Re: Review Request 36436: Prevent job updates from allowing unbounded instance events
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36436/ --- (Updated July 15, 2015, 1:23 p.m.) Review request for Aurora and Bill Farner. Bugs: AURORA-1096 https://issues.apache.org/jira/browse/AURORA-1096 Repository: aurora Description --- Prevent job updates from allowing unbounded instance events Diffs (updated) - src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java dc0cd2d71d11b8157154f7b63a14f0282dee09f1 src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java d28baba7618ebb194a61455971786aef46abd8eb Diff: https://reviews.apache.org/r/36436/diff/ Testing --- `./gradlew build -Pq` Thanks, Joe Smith
Re: Review Request 36436: Prevent job updates from allowing unbounded instance events
On July 15, 2015, 11:47 a.m., Bill Farner wrote: src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java, lines 168-169 https://reviews.apache.org/r/36436/diff/4/?file=1012829#file1012829line168 Sorry, i made that suggestion before i added the comment to break out constants and multiply. Feel free to nuke this :-/ No worries at all! I found this comment to still be ~helpful, but I can see how the constants make this redundant and thus removable. Thanks again! :) - Joe --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36436/#review91790 --- On July 15, 2015, 11:08 a.m., Joe Smith wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36436/ --- (Updated July 15, 2015, 11:08 a.m.) Review request for Aurora and Bill Farner. Bugs: AURORA-1096 https://issues.apache.org/jira/browse/AURORA-1096 Repository: aurora Description --- Prevent job updates from allowing unbounded instance events Diffs - src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java dc0cd2d71d11b8157154f7b63a14f0282dee09f1 src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java d28baba7618ebb194a61455971786aef46abd8eb Diff: https://reviews.apache.org/r/36436/diff/ Testing --- `./gradlew build -Pq` Thanks, Joe Smith
Re: Review Request 36436: Prevent job updates from allowing unbounded instance events
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36436/#review91701 --- Ship it! Master (d9dac92) 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 July 15, 2015, 12:30 a.m., Joe Smith wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36436/ --- (Updated July 15, 2015, 12:30 a.m.) Review request for Aurora and Bill Farner. Bugs: AURORA-1096 https://issues.apache.org/jira/browse/AURORA-1096 Repository: aurora Description --- Prevent job updates from allowing unbounded instance events Diffs - src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java dc0cd2d71d11b8157154f7b63a14f0282dee09f1 src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java d28baba7618ebb194a61455971786aef46abd8eb Diff: https://reviews.apache.org/r/36436/diff/ Testing --- `./gradlew build -Pq` Thanks, Joe Smith
Re: Review Request 36436: Prevent job updates from allowing unbounded instance events
On July 14, 2015, 10:55 a.m., Bill Farner wrote: src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java, lines 168-177 https://reviews.apache.org/r/36436/diff/2/?file=1010206#file1010206line168 Have you considered a single arg for the upper bound of instance events that are allowed for a job update? I think this would be easier for the operator to reason about, and simpler to associate directly to DB impact. Good idea, and much simpler. Let me know what you think of the implementation. Thanks! - Joe --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36436/#review91645 --- On July 13, 2015, 4:10 p.m., Joe Smith wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36436/ --- (Updated July 13, 2015, 4:10 p.m.) Review request for Aurora and Bill Farner. Bugs: AURORA-1096 https://issues.apache.org/jira/browse/AURORA-1096 Repository: aurora Description --- Prevent job updates from allowing unbounded instance events Diffs - src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java dc0cd2d71d11b8157154f7b63a14f0282dee09f1 src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java d28baba7618ebb194a61455971786aef46abd8eb Diff: https://reviews.apache.org/r/36436/diff/ Testing --- `./gradlew build -Pq` Thanks, Joe Smith
Re: Review Request 36436: Prevent job updates from allowing unbounded instance events
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36436/ --- (Updated July 14, 2015, 5:30 p.m.) Review request for Aurora and Bill Farner. Bugs: AURORA-1096 https://issues.apache.org/jira/browse/AURORA-1096 Repository: aurora Description --- Prevent job updates from allowing unbounded instance events Diffs (updated) - src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java dc0cd2d71d11b8157154f7b63a14f0282dee09f1 src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java d28baba7618ebb194a61455971786aef46abd8eb Diff: https://reviews.apache.org/r/36436/diff/ Testing --- `./gradlew build -Pq` Thanks, Joe Smith
Re: Review Request 36436: Prevent job updates from allowing unbounded instance events
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36436/#review91645 --- src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java (lines 168 - 177) https://reviews.apache.org/r/36436/#comment145170 Have you considered a single arg for the upper bound of instance events that are allowed for a job update? I think this would be easier for the operator to reason about, and simpler to associate directly to DB impact. - Bill Farner On July 13, 2015, 11:10 p.m., Joe Smith wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36436/ --- (Updated July 13, 2015, 11:10 p.m.) Review request for Aurora and Bill Farner. Bugs: AURORA-1096 https://issues.apache.org/jira/browse/AURORA-1096 Repository: aurora Description --- Prevent job updates from allowing unbounded instance events Diffs - src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java dc0cd2d71d11b8157154f7b63a14f0282dee09f1 src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java d28baba7618ebb194a61455971786aef46abd8eb Diff: https://reviews.apache.org/r/36436/diff/ Testing --- `./gradlew build -Pq` Thanks, Joe Smith
Re: Review Request 36436: Prevent job updates from allowing unbounded instance events
On July 12, 2015, 11:38 p.m., Stephan Erb wrote: src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java, line 169 https://reviews.apache.org/r/36436/diff/1/?file=1009204#file1009204line169 Give that this will be the only documentation of this feature, the doc string could be a bit longer and explain that this option applies to the job update feature and limits the `max_per_shard_failures` jobspec configuration. Done (I believe, thatis. Let me know if you think this is clearer!) On July 12, 2015, 11:38 p.m., Stephan Erb wrote: src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java, line 173 https://reviews.apache.org/r/36436/diff/1/?file=1009204#file1009204line173 Same as above. It is hard to understand the implications of this option without reading the code. Done. (But let me know what you think!) - Joe --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36436/#review91416 --- On July 12, 2015, 11:49 a.m., Joe Smith wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36436/ --- (Updated July 12, 2015, 11:49 a.m.) Review request for Aurora and Bill Farner. Bugs: AURORA-1096 https://issues.apache.org/jira/browse/AURORA-1096 Repository: aurora Description --- Prevent job updates from allowing unbounded instance events Diffs - src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java dc0cd2d71d11b8157154f7b63a14f0282dee09f1 src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java d28baba7618ebb194a61455971786aef46abd8eb Diff: https://reviews.apache.org/r/36436/diff/ Testing --- `./gradlew build -Pq` Thanks, Joe Smith
