You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@aurora.apache.org by Maxim Khutornenko <ma...@apache.org> on 2014/09/20 01:08:05 UTC
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/
-----------------------------------------------------------
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.
Posted by Maxim Khutornenko <ma...@apache.org>.
> On Sept. 20, 2014, 2:14 a.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java, line 1375
> > <https://reviews.apache.org/r/25857/diff/2/?file=698338#file698338line1375>
> >
> > How about:
> >
> > return addMessage(
> > emptyResponse(),
> > INVALID_REQUEST,
> > "Server-side updates are disabled on this cluster.");
Done.
> 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.
>
> Bill Farner wrote:
> 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.
Refactored.
> On Sept. 20, 2014, 2:14 a.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java, line 251
> > <https://reviews.apache.org/r/25857/diff/2/?file=698338#file698338line251>
> >
> > While you're here, can you remove this constructor overload? It's unused.
Done.
> On Sept. 20, 2014, 2:14 a.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java, line 188
> > <https://reviews.apache.org/r/25857/diff/2/?file=698338#file698338line188>
> >
> > "Enable the async updater. Use at your own risk, this feature is not yet complete."
Done.
> On Sept. 20, 2014, 2:14 a.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java, line 187
> > <https://reviews.apache.org/r/25857/diff/2/?file=698338#file698338line187>
> >
> > How about `enable_beta_updater`
Done.
- 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.
Posted by Bill Farner <wf...@apache.org>.
> 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.
Posted by Maxim Khutornenko <ma...@apache.org>.
> 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.
Posted by Bill Farner <wf...@apache.org>.
-----------------------------------------------------------
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
>
>
Re: Review Request 25857: Disable updater by default in prod.
Posted by Bill Farner <wf...@apache.org>.
-----------------------------------------------------------
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.
Posted by Maxim Khutornenko <ma...@apache.org>.
-----------------------------------------------------------
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.
Posted by Maxim Khutornenko <ma...@apache.org>.
-----------------------------------------------------------
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.
Changes
-------
CR comments.
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.
Posted by Maxim Khutornenko <ma...@apache.org>.
-----------------------------------------------------------
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