Re: Review Request 32106: Changed the updater to not update an instance if only the job owner changes
On March 16, 2015, 4:30 p.m., Maxim Khutornenko wrote: src/main/java/org/apache/aurora/scheduler/updater/InstanceUpdater.java, line 159 https://reviews.apache.org/r/32106/diff/1/?file=895820#file895820line159 There is actually a better place to do this type of action [1]. If not done there, the update will still have to iterate over all instances and generate a lot of instance event noise. Also, the UI will show these instances in the update working set, which does not reflect the NOOP nature of this action. [1] - https://github.com/apache/incubator-aurora/blob/master/src/main/java/org/apache/aurora/scheduler/updater/JobDiff.java Bill Farner wrote: Good point. Steve landed at this line based on my hasty direction. You're right, though, that JobDiff is the place to move it. Perhaps this comparison should be done in JobDiff as well, to prevent impedance mismatch? Also, better yet - a test case in `JobUpdaterIT` should have pointed out this flaw. Steve - can you add a case there? - Bill --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32106/#review76568 --- On March 16, 2015, 3:13 p.m., Steve Niemitz wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32106/ --- (Updated March 16, 2015, 3:13 p.m.) Review request for Aurora and Bill Farner. Bugs: AURORA-1200 https://issues.apache.org/jira/browse/AURORA-1200 Repository: aurora Description --- Changed the updater to not update an instance if the job owner changes Diffs - src/main/java/org/apache/aurora/scheduler/updater/InstanceUpdater.java 14753cf5ef35d5133ca5029f3a465df884756070 src/test/java/org/apache/aurora/scheduler/updater/InstanceUpdaterTest.java 09c147e76d7c2c130a1fdd85459c45395fee7dde Diff: https://reviews.apache.org/r/32106/diff/ Testing --- Thanks, Steve Niemitz
Re: Review Request 32106: Changed the updater to not update an instance if only the job owner changes
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32106/#review76568 --- src/main/java/org/apache/aurora/scheduler/updater/InstanceUpdater.java https://reviews.apache.org/r/32106/#comment124146 There is actually a better place to do this type of action [1]. If not done there, the update will still have to iterate over all instances and generate a lot of instance event noise. Also, the UI will show these instances in the update working set, which does not reflect the NOOP nature of this action. [1] - https://github.com/apache/incubator-aurora/blob/master/src/main/java/org/apache/aurora/scheduler/updater/JobDiff.java - Maxim Khutornenko On March 16, 2015, 3:13 p.m., Steve Niemitz wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32106/ --- (Updated March 16, 2015, 3:13 p.m.) Review request for Aurora and Bill Farner. Bugs: AURORA-1200 https://issues.apache.org/jira/browse/AURORA-1200 Repository: aurora Description --- Changed the updater to not update an instance if the job owner changes Diffs - src/main/java/org/apache/aurora/scheduler/updater/InstanceUpdater.java 14753cf5ef35d5133ca5029f3a465df884756070 src/test/java/org/apache/aurora/scheduler/updater/InstanceUpdaterTest.java 09c147e76d7c2c130a1fdd85459c45395fee7dde Diff: https://reviews.apache.org/r/32106/diff/ Testing --- Thanks, Steve Niemitz
Re: Review Request 32106: Changed the updater to not update an instance if only the job owner changes
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32106/#review76560 --- Ship it! Master (e11ed5b) 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 March 16, 2015, 3:13 p.m., Steve Niemitz wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32106/ --- (Updated March 16, 2015, 3:13 p.m.) Review request for Aurora and Bill Farner. Bugs: AURORA-1200 https://issues.apache.org/jira/browse/AURORA-1200 Repository: aurora Description --- Changed the updater to not update an instance if the job owner changes Diffs - src/main/java/org/apache/aurora/scheduler/updater/InstanceUpdater.java 14753cf5ef35d5133ca5029f3a465df884756070 src/test/java/org/apache/aurora/scheduler/updater/InstanceUpdaterTest.java 09c147e76d7c2c130a1fdd85459c45395fee7dde Diff: https://reviews.apache.org/r/32106/diff/ Testing --- Thanks, Steve Niemitz
Review Request 32106: Changed the updater to not update an instance if only the job owner changes
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32106/ --- Review request for Aurora and Bill Farner. Bugs: AURORA-1200 https://issues.apache.org/jira/browse/AURORA-1200 Repository: aurora Description --- Changed the updater to not update an instance if the job owner changes Diffs - src/main/java/org/apache/aurora/scheduler/updater/InstanceUpdater.java 14753cf5ef35d5133ca5029f3a465df884756070 src/test/java/org/apache/aurora/scheduler/updater/InstanceUpdaterTest.java 09c147e76d7c2c130a1fdd85459c45395fee7dde Diff: https://reviews.apache.org/r/32106/diff/ Testing --- Thanks, Steve Niemitz
Re: Review Request 32106: Changed the updater to not update an instance if only the job owner changes
On March 16, 2015, 4:30 p.m., Maxim Khutornenko wrote: src/main/java/org/apache/aurora/scheduler/updater/InstanceUpdater.java, line 159 https://reviews.apache.org/r/32106/diff/1/?file=895820#file895820line159 There is actually a better place to do this type of action [1]. If not done there, the update will still have to iterate over all instances and generate a lot of instance event noise. Also, the UI will show these instances in the update working set, which does not reflect the NOOP nature of this action. [1] - https://github.com/apache/incubator-aurora/blob/master/src/main/java/org/apache/aurora/scheduler/updater/JobDiff.java Good point. Steve landed at this line based on my hasty direction. You're right, though, that JobDiff is the place to move it. Perhaps this comparison should be done in JobDiff as well, to prevent impedance mismatch? - Bill --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32106/#review76568 --- On March 16, 2015, 3:13 p.m., Steve Niemitz wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32106/ --- (Updated March 16, 2015, 3:13 p.m.) Review request for Aurora and Bill Farner. Bugs: AURORA-1200 https://issues.apache.org/jira/browse/AURORA-1200 Repository: aurora Description --- Changed the updater to not update an instance if the job owner changes Diffs - src/main/java/org/apache/aurora/scheduler/updater/InstanceUpdater.java 14753cf5ef35d5133ca5029f3a465df884756070 src/test/java/org/apache/aurora/scheduler/updater/InstanceUpdaterTest.java 09c147e76d7c2c130a1fdd85459c45395fee7dde Diff: https://reviews.apache.org/r/32106/diff/ Testing --- Thanks, Steve Niemitz
