Re: Review Request 25857: Disable updater by default in prod.
On Sept. 20, 2014, 2:14 a.m., Bill Farner wrote: src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java, line 286 https://reviews.apache.org/r/25857/diff/2/?file=698339#file698339line286 I see why you went this route, but it's a precedent i'd rather not set. How about a guice-bound `@EnableUpdater Boolean`. Then, create a new test class that does nothing but create necessary mocks to construct an instance of SchedulerThriftInterface and test the flag. This will result in a much larger diff for something that is only used temporarily and will go away once all updater kinks are worked out. I'd rather go with a smaller diff and document it as testing anti-pattern not to be used for anything permanent. - Maxim --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25857/#review54071 --- On Sept. 19, 2014, 11:10 p.m., Maxim Khutornenko wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25857/ --- (Updated Sept. 19, 2014, 11:10 p.m.) Review request for Aurora and Bill Farner. Bugs: AURORA-732 https://issues.apache.org/jira/browse/AURORA-732 Repository: aurora Description --- Added command line arg to control startJobUpdate. Diffs - src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java 83ac034cff009530e5e16c0613b9d085f3b908d8 src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java 336cada625b85618486660fc24f3e83a312609f8 Diff: https://reviews.apache.org/r/25857/diff/ Testing --- gradle -Pq build Thanks, Maxim Khutornenko
Re: Review Request 25857: Disable updater by default in prod.
On Sept. 20, 2014, 2:14 a.m., Bill Farner wrote: src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java, line 286 https://reviews.apache.org/r/25857/diff/2/?file=698339#file698339line286 I see why you went this route, but it's a precedent i'd rather not set. How about a guice-bound `@EnableUpdater Boolean`. Then, create a new test class that does nothing but create necessary mocks to construct an instance of SchedulerThriftInterface and test the flag. Maxim Khutornenko wrote: This will result in a much larger diff for something that is only used temporarily and will go away once all updater kinks are worked out. I'd rather go with a smaller diff and document it as testing anti-pattern not to be used for anything permanent. A fair argument, but i'm still a big anti-fan of using reflection like this. I'd much rather the field be public than have false encapsulation. - Bill --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25857/#review54071 --- On Sept. 19, 2014, 11:10 p.m., Maxim Khutornenko wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25857/ --- (Updated Sept. 19, 2014, 11:10 p.m.) Review request for Aurora and Bill Farner. Bugs: AURORA-732 https://issues.apache.org/jira/browse/AURORA-732 Repository: aurora Description --- Added command line arg to control startJobUpdate. Diffs - src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java 83ac034cff009530e5e16c0613b9d085f3b908d8 src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java 336cada625b85618486660fc24f3e83a312609f8 Diff: https://reviews.apache.org/r/25857/diff/ Testing --- gradle -Pq build Thanks, Maxim Khutornenko
Re: Review Request 25857: Disable updater by default in prod.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25857/#review54216 --- Ship it! Thanks! - Bill Farner On Sept. 22, 2014, 8:41 p.m., Maxim Khutornenko wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25857/ --- (Updated Sept. 22, 2014, 8:41 p.m.) Review request for Aurora and Bill Farner. Bugs: AURORA-732 https://issues.apache.org/jira/browse/AURORA-732 Repository: aurora Description --- Added command line arg to control startJobUpdate. Diffs - src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java 83ac034cff009530e5e16c0613b9d085f3b908d8 src/main/java/org/apache/aurora/scheduler/thrift/ThriftModule.java aa35182c47582fbd803a277cd4fde4a34a9dcc2a src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java 336cada625b85618486660fc24f3e83a312609f8 Diff: https://reviews.apache.org/r/25857/diff/ Testing --- gradle -Pq build Thanks, Maxim Khutornenko
Re: Review Request 25857: Disable updater by default in prod.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25857/ --- (Updated Sept. 23, 2014, 1:12 a.m.) Review request for Aurora and Bill Farner. Changes --- Replacing @BingingAnnotation with @Qualifier to match master. Bugs: AURORA-732 https://issues.apache.org/jira/browse/AURORA-732 Repository: aurora Description --- Added command line arg to control startJobUpdate. Diffs (updated) - src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java 83ac034cff009530e5e16c0613b9d085f3b908d8 src/main/java/org/apache/aurora/scheduler/thrift/ThriftModule.java aa35182c47582fbd803a277cd4fde4a34a9dcc2a src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java 336cada625b85618486660fc24f3e83a312609f8 Diff: https://reviews.apache.org/r/25857/diff/ Testing --- gradle -Pq build Thanks, Maxim Khutornenko
Re: Review Request 25857: Disable updater by default in prod.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25857/ --- (Updated Sept. 19, 2014, 11:10 p.m.) Review request for Aurora and Bill Farner. Bugs: AURORA-732 https://issues.apache.org/jira/browse/AURORA-732 Repository: aurora Description --- Added command line arg to control startJobUpdate. Diffs (updated) - src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java 83ac034cff009530e5e16c0613b9d085f3b908d8 src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java 336cada625b85618486660fc24f3e83a312609f8 Diff: https://reviews.apache.org/r/25857/diff/ Testing --- gradle -Pq build Thanks, Maxim Khutornenko
Re: Review Request 25857: Disable updater by default in prod.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25857/#review54071 --- src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java https://reviews.apache.org/r/25857/#comment94020 How about `enable_beta_updater` src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java https://reviews.apache.org/r/25857/#comment94021 Enable the async updater. Use at your own risk, this feature is not yet complete. src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java https://reviews.apache.org/r/25857/#comment94019 While you're here, can you remove this constructor overload? It's unused. src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java https://reviews.apache.org/r/25857/#comment94018 How about: return addMessage( emptyResponse(), INVALID_REQUEST, Server-side updates are disabled on this cluster.); src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java https://reviews.apache.org/r/25857/#comment94022 I see why you went this route, but it's a precedent i'd rather not set. How about a guice-bound `@EnableUpdater Boolean`. Then, create a new test class that does nothing but create necessary mocks to construct an instance of SchedulerThriftInterface and test the flag. - Bill Farner On Sept. 19, 2014, 11:10 p.m., Maxim Khutornenko wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25857/ --- (Updated Sept. 19, 2014, 11:10 p.m.) Review request for Aurora and Bill Farner. Bugs: AURORA-732 https://issues.apache.org/jira/browse/AURORA-732 Repository: aurora Description --- Added command line arg to control startJobUpdate. Diffs - src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java 83ac034cff009530e5e16c0613b9d085f3b908d8 src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java 336cada625b85618486660fc24f3e83a312609f8 Diff: https://reviews.apache.org/r/25857/diff/ Testing --- gradle -Pq build Thanks, Maxim Khutornenko
