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 2015/08/27 03:07:35 UTC
Review Request 37825: Adding minimal implementation of the external
tier config.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37825/
-----------------------------------------------------------
Review request for Aurora, Bill Farner and Zameer Manji.
Bugs: AURORA-1437
https://issues.apache.org/jira/browse/AURORA-1437
Repository: aurora
Description
-------
The external config file is optional for now as tiers are not fully defined yet.
Diffs
-----
examples/vagrant/upstart/aurora-scheduler-kerberos.conf 744b4a35c61e749734e222b3d4cbd296927665aa
examples/vagrant/upstart/aurora-scheduler.conf 789a3a0315e8530880999432aa9b1e7d0f57d1ff
src/main/java/org/apache/aurora/scheduler/SchedulerModule.java d4bc6b9bb10c982fb6a2458d7fdd12bddbe98eca
src/main/java/org/apache/aurora/scheduler/TierManager.java 652afece17a7eb09b0ca68066707b1b8fbf024f0
src/test/java/org/apache/aurora/scheduler/TierManagerTest.java eb0207114e1b93968cb65832f154a9cd3bc3232e
src/test/resources/org/apache/aurora/scheduler/tiers-example.json PRE-CREATION
Diff: https://reviews.apache.org/r/37825/diff/
Testing
-------
./gradlew -Pq build
./src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh
Thanks,
Maxim Khutornenko
Re: Review Request 37825: Adding minimal implementation of the
external tier config.
Posted by Bill Farner <wf...@apache.org>.
> On Aug. 27, 2015, 10:23 a.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/TierManager.java, line 63
> > <https://reviews.apache.org/r/37825/diff/1/?file=1055551#file1055551line63>
> >
> > Consider moving the parsing up a layer, in the module. That way we can report earlier if the file is malformed, and you can bind a specific type (without the annotation).
> >
> > This will also address another complaint, which is that the config file is being parsed every time we want to find the tier a task config belongs to.
> This will also address another complaint, which is that the config file is being parsed every time we want to find the tier a task config belongs to.'
Sorry, this was a lie. Please ignore.
- Bill
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37825/#review96703
-----------------------------------------------------------
On Aug. 26, 2015, 6:07 p.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37825/
> -----------------------------------------------------------
>
> (Updated Aug. 26, 2015, 6:07 p.m.)
>
>
> Review request for Aurora, Bill Farner and Zameer Manji.
>
>
> Bugs: AURORA-1437
> https://issues.apache.org/jira/browse/AURORA-1437
>
>
> Repository: aurora
>
>
> Description
> -------
>
> The external config file is optional for now as tiers are not fully defined yet.
>
>
> Diffs
> -----
>
> examples/vagrant/upstart/aurora-scheduler-kerberos.conf 744b4a35c61e749734e222b3d4cbd296927665aa
> examples/vagrant/upstart/aurora-scheduler.conf 789a3a0315e8530880999432aa9b1e7d0f57d1ff
> src/main/java/org/apache/aurora/scheduler/SchedulerModule.java d4bc6b9bb10c982fb6a2458d7fdd12bddbe98eca
> src/main/java/org/apache/aurora/scheduler/TierManager.java 652afece17a7eb09b0ca68066707b1b8fbf024f0
> src/test/java/org/apache/aurora/scheduler/TierManagerTest.java eb0207114e1b93968cb65832f154a9cd3bc3232e
> src/test/resources/org/apache/aurora/scheduler/tiers-example.json PRE-CREATION
>
> Diff: https://reviews.apache.org/r/37825/diff/
>
>
> Testing
> -------
>
> ./gradlew -Pq build
> ./src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh
>
>
> Thanks,
>
> Maxim Khutornenko
>
>
Re: Review Request 37825: Adding minimal implementation of the
external tier config.
Posted by Maxim Khutornenko <ma...@apache.org>.
> On Aug. 27, 2015, 5:23 p.m., Bill Farner wrote:
> > src/test/resources/org/apache/aurora/scheduler/tiers-example.json, line 2
> > <https://reviews.apache.org/r/37825/diff/1/?file=1055553#file1055553line2>
> >
> > The added layer here seems slightly weird, but i can see why it's useful - you can add other fields to the config in the future. I don't necessarily expect you to change anything, but i'm curious if that was your intention.
>
> Zameer Manji wrote:
> This also seems weird to me. I think a good schema for the file would be { <tier name>: <tier properties> }. Maybe this requires a mailing list discussion?
Bill was on target with his guess. The extra layer does not add much overhead and at the same time does not prevent adding extra metadata in future. This is the same logical structuring proposed in the design doc.
- Maxim
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37825/#review96703
-----------------------------------------------------------
On Aug. 27, 2015, 1:07 a.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37825/
> -----------------------------------------------------------
>
> (Updated Aug. 27, 2015, 1:07 a.m.)
>
>
> Review request for Aurora, Bill Farner and Zameer Manji.
>
>
> Bugs: AURORA-1437
> https://issues.apache.org/jira/browse/AURORA-1437
>
>
> Repository: aurora
>
>
> Description
> -------
>
> The external config file is optional for now as tiers are not fully defined yet.
>
>
> Diffs
> -----
>
> examples/vagrant/upstart/aurora-scheduler-kerberos.conf 744b4a35c61e749734e222b3d4cbd296927665aa
> examples/vagrant/upstart/aurora-scheduler.conf 789a3a0315e8530880999432aa9b1e7d0f57d1ff
> src/main/java/org/apache/aurora/scheduler/SchedulerModule.java d4bc6b9bb10c982fb6a2458d7fdd12bddbe98eca
> src/main/java/org/apache/aurora/scheduler/TierManager.java 652afece17a7eb09b0ca68066707b1b8fbf024f0
> src/test/java/org/apache/aurora/scheduler/TierManagerTest.java eb0207114e1b93968cb65832f154a9cd3bc3232e
> src/test/resources/org/apache/aurora/scheduler/tiers-example.json PRE-CREATION
>
> Diff: https://reviews.apache.org/r/37825/diff/
>
>
> Testing
> -------
>
> ./gradlew -Pq build
> ./src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh
>
>
> Thanks,
>
> Maxim Khutornenko
>
>
Re: Review Request 37825: Adding minimal implementation of the
external tier config.
Posted by Maxim Khutornenko <ma...@apache.org>.
> On Aug. 27, 2015, 5:23 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/TierManager.java, line 63
> > <https://reviews.apache.org/r/37825/diff/1/?file=1055551#file1055551line63>
> >
> > Consider moving the parsing up a layer, in the module. That way we can report earlier if the file is malformed, and you can bind a specific type (without the annotation).
> >
> > This will also address another complaint, which is that the config file is being parsed every time we want to find the tier a task config belongs to.
>
> Bill Farner wrote:
> > This will also address another complaint, which is that the config file is being parsed every time we want to find the tier a task config belongs to.'
>
> Sorry, this was a lie. Please ignore.
Good point. This was done mainly for testability but nothing prevents testing it in the module either.
> On Aug. 27, 2015, 5:23 p.m., Bill Farner wrote:
> > src/test/resources/org/apache/aurora/scheduler/tiers-example.json, line 2
> > <https://reviews.apache.org/r/37825/diff/1/?file=1055553#file1055553line2>
> >
> > The added layer here seems slightly weird, but i can see why it's useful - you can add other fields to the config in the future. I don't necessarily expect you to change anything, but i'm curious if that was your intention.
>
> Zameer Manji wrote:
> This also seems weird to me. I think a good schema for the file would be { <tier name>: <tier properties> }. Maybe this requires a mailing list discussion?
>
> Maxim Khutornenko wrote:
> Bill was on target with his guess. The extra layer does not add much overhead and at the same time does not prevent adding extra metadata in future. This is the same logical structuring proposed in the design doc.
>
> Bill Farner wrote:
> I'm all for discussion on the mailing list, but i think this is minutia that is safe to define here and avoid the overhead of outreach/waiting.
+1. Given that the structure has been laid down in the design doc, I'd like to proceed with this schema and reserve dev for heavier weight discussions.
> On Aug. 27, 2015, 5:23 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/SchedulerModule.java, line 70
> > <https://reviews.apache.org/r/37825/diff/1/?file=1055550#file1055550line70>
> >
> > +@CanRead
>
> Kevin Sweeney wrote:
> That will cause loading to fail when the argument is null.
Exactly. Added a comment in the TODO to address it later.
- Maxim
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37825/#review96703
-----------------------------------------------------------
On Aug. 27, 2015, 1:07 a.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37825/
> -----------------------------------------------------------
>
> (Updated Aug. 27, 2015, 1:07 a.m.)
>
>
> Review request for Aurora, Bill Farner and Zameer Manji.
>
>
> Bugs: AURORA-1437
> https://issues.apache.org/jira/browse/AURORA-1437
>
>
> Repository: aurora
>
>
> Description
> -------
>
> The external config file is optional for now as tiers are not fully defined yet.
>
>
> Diffs
> -----
>
> examples/vagrant/upstart/aurora-scheduler-kerberos.conf 744b4a35c61e749734e222b3d4cbd296927665aa
> examples/vagrant/upstart/aurora-scheduler.conf 789a3a0315e8530880999432aa9b1e7d0f57d1ff
> src/main/java/org/apache/aurora/scheduler/SchedulerModule.java d4bc6b9bb10c982fb6a2458d7fdd12bddbe98eca
> src/main/java/org/apache/aurora/scheduler/TierManager.java 652afece17a7eb09b0ca68066707b1b8fbf024f0
> src/test/java/org/apache/aurora/scheduler/TierManagerTest.java eb0207114e1b93968cb65832f154a9cd3bc3232e
> src/test/resources/org/apache/aurora/scheduler/tiers-example.json PRE-CREATION
>
> Diff: https://reviews.apache.org/r/37825/diff/
>
>
> Testing
> -------
>
> ./gradlew -Pq build
> ./src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh
>
>
> Thanks,
>
> Maxim Khutornenko
>
>
Re: Review Request 37825: Adding minimal implementation of the
external tier config.
Posted by Bill Farner <wf...@apache.org>.
> On Aug. 27, 2015, 10:23 a.m., Bill Farner wrote:
> > src/test/resources/org/apache/aurora/scheduler/tiers-example.json, line 2
> > <https://reviews.apache.org/r/37825/diff/1/?file=1055553#file1055553line2>
> >
> > The added layer here seems slightly weird, but i can see why it's useful - you can add other fields to the config in the future. I don't necessarily expect you to change anything, but i'm curious if that was your intention.
>
> Zameer Manji wrote:
> This also seems weird to me. I think a good schema for the file would be { <tier name>: <tier properties> }. Maybe this requires a mailing list discussion?
>
> Maxim Khutornenko wrote:
> Bill was on target with his guess. The extra layer does not add much overhead and at the same time does not prevent adding extra metadata in future. This is the same logical structuring proposed in the design doc.
I'm all for discussion on the mailing list, but i think this is minutia that is safe to define here and avoid the overhead of outreach/waiting.
- Bill
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37825/#review96703
-----------------------------------------------------------
On Aug. 26, 2015, 6:07 p.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37825/
> -----------------------------------------------------------
>
> (Updated Aug. 26, 2015, 6:07 p.m.)
>
>
> Review request for Aurora, Bill Farner and Zameer Manji.
>
>
> Bugs: AURORA-1437
> https://issues.apache.org/jira/browse/AURORA-1437
>
>
> Repository: aurora
>
>
> Description
> -------
>
> The external config file is optional for now as tiers are not fully defined yet.
>
>
> Diffs
> -----
>
> examples/vagrant/upstart/aurora-scheduler-kerberos.conf 744b4a35c61e749734e222b3d4cbd296927665aa
> examples/vagrant/upstart/aurora-scheduler.conf 789a3a0315e8530880999432aa9b1e7d0f57d1ff
> src/main/java/org/apache/aurora/scheduler/SchedulerModule.java d4bc6b9bb10c982fb6a2458d7fdd12bddbe98eca
> src/main/java/org/apache/aurora/scheduler/TierManager.java 652afece17a7eb09b0ca68066707b1b8fbf024f0
> src/test/java/org/apache/aurora/scheduler/TierManagerTest.java eb0207114e1b93968cb65832f154a9cd3bc3232e
> src/test/resources/org/apache/aurora/scheduler/tiers-example.json PRE-CREATION
>
> Diff: https://reviews.apache.org/r/37825/diff/
>
>
> Testing
> -------
>
> ./gradlew -Pq build
> ./src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh
>
>
> Thanks,
>
> Maxim Khutornenko
>
>
Re: Review Request 37825: Adding minimal implementation of the
external tier config.
Posted by Kevin Sweeney <ke...@apache.org>.
> On Aug. 27, 2015, 10:23 a.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/SchedulerModule.java, line 70
> > <https://reviews.apache.org/r/37825/diff/1/?file=1055550#file1055550line70>
> >
> > +@CanRead
That will cause loading to fail when the argument is null.
- Kevin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37825/#review96703
-----------------------------------------------------------
On Aug. 26, 2015, 6:07 p.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37825/
> -----------------------------------------------------------
>
> (Updated Aug. 26, 2015, 6:07 p.m.)
>
>
> Review request for Aurora, Bill Farner and Zameer Manji.
>
>
> Bugs: AURORA-1437
> https://issues.apache.org/jira/browse/AURORA-1437
>
>
> Repository: aurora
>
>
> Description
> -------
>
> The external config file is optional for now as tiers are not fully defined yet.
>
>
> Diffs
> -----
>
> examples/vagrant/upstart/aurora-scheduler-kerberos.conf 744b4a35c61e749734e222b3d4cbd296927665aa
> examples/vagrant/upstart/aurora-scheduler.conf 789a3a0315e8530880999432aa9b1e7d0f57d1ff
> src/main/java/org/apache/aurora/scheduler/SchedulerModule.java d4bc6b9bb10c982fb6a2458d7fdd12bddbe98eca
> src/main/java/org/apache/aurora/scheduler/TierManager.java 652afece17a7eb09b0ca68066707b1b8fbf024f0
> src/test/java/org/apache/aurora/scheduler/TierManagerTest.java eb0207114e1b93968cb65832f154a9cd3bc3232e
> src/test/resources/org/apache/aurora/scheduler/tiers-example.json PRE-CREATION
>
> Diff: https://reviews.apache.org/r/37825/diff/
>
>
> Testing
> -------
>
> ./gradlew -Pq build
> ./src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh
>
>
> Thanks,
>
> Maxim Khutornenko
>
>
Re: Review Request 37825: Adding minimal implementation of the
external tier config.
Posted by Zameer Manji <zm...@apache.org>.
> On Aug. 27, 2015, 10:23 a.m., Bill Farner wrote:
> > src/test/resources/org/apache/aurora/scheduler/tiers-example.json, line 2
> > <https://reviews.apache.org/r/37825/diff/1/?file=1055553#file1055553line2>
> >
> > The added layer here seems slightly weird, but i can see why it's useful - you can add other fields to the config in the future. I don't necessarily expect you to change anything, but i'm curious if that was your intention.
This also seems weird to me. I think a good schema for the file would be { <tier name>: <tier properties> }. Maybe this requires a mailing list discussion?
- Zameer
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37825/#review96703
-----------------------------------------------------------
On Aug. 26, 2015, 6:07 p.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37825/
> -----------------------------------------------------------
>
> (Updated Aug. 26, 2015, 6:07 p.m.)
>
>
> Review request for Aurora, Bill Farner and Zameer Manji.
>
>
> Bugs: AURORA-1437
> https://issues.apache.org/jira/browse/AURORA-1437
>
>
> Repository: aurora
>
>
> Description
> -------
>
> The external config file is optional for now as tiers are not fully defined yet.
>
>
> Diffs
> -----
>
> examples/vagrant/upstart/aurora-scheduler-kerberos.conf 744b4a35c61e749734e222b3d4cbd296927665aa
> examples/vagrant/upstart/aurora-scheduler.conf 789a3a0315e8530880999432aa9b1e7d0f57d1ff
> src/main/java/org/apache/aurora/scheduler/SchedulerModule.java d4bc6b9bb10c982fb6a2458d7fdd12bddbe98eca
> src/main/java/org/apache/aurora/scheduler/TierManager.java 652afece17a7eb09b0ca68066707b1b8fbf024f0
> src/test/java/org/apache/aurora/scheduler/TierManagerTest.java eb0207114e1b93968cb65832f154a9cd3bc3232e
> src/test/resources/org/apache/aurora/scheduler/tiers-example.json PRE-CREATION
>
> Diff: https://reviews.apache.org/r/37825/diff/
>
>
> Testing
> -------
>
> ./gradlew -Pq build
> ./src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh
>
>
> Thanks,
>
> Maxim Khutornenko
>
>
Re: Review Request 37825: Adding minimal implementation of the
external tier config.
Posted by Bill Farner <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37825/#review96703
-----------------------------------------------------------
src/main/java/org/apache/aurora/scheduler/SchedulerModule.java (line 70)
<https://reviews.apache.org/r/37825/#comment152332>
+@CanRead
src/main/java/org/apache/aurora/scheduler/TierManager.java (line 62)
<https://reviews.apache.org/r/37825/#comment152333>
Consider moving the parsing up a layer, in the module. That way we can report earlier if the file is malformed, and you can bind a specific type (without the annotation).
This will also address another complaint, which is that the config file is being parsed every time we want to find the tier a task config belongs to.
src/test/resources/org/apache/aurora/scheduler/tiers-example.json (line 2)
<https://reviews.apache.org/r/37825/#comment152334>
The added layer here seems slightly weird, but i can see why it's useful - you can add other fields to the config in the future. I don't necessarily expect you to change anything, but i'm curious if that was your intention.
- Bill Farner
On Aug. 26, 2015, 6:07 p.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37825/
> -----------------------------------------------------------
>
> (Updated Aug. 26, 2015, 6:07 p.m.)
>
>
> Review request for Aurora, Bill Farner and Zameer Manji.
>
>
> Bugs: AURORA-1437
> https://issues.apache.org/jira/browse/AURORA-1437
>
>
> Repository: aurora
>
>
> Description
> -------
>
> The external config file is optional for now as tiers are not fully defined yet.
>
>
> Diffs
> -----
>
> examples/vagrant/upstart/aurora-scheduler-kerberos.conf 744b4a35c61e749734e222b3d4cbd296927665aa
> examples/vagrant/upstart/aurora-scheduler.conf 789a3a0315e8530880999432aa9b1e7d0f57d1ff
> src/main/java/org/apache/aurora/scheduler/SchedulerModule.java d4bc6b9bb10c982fb6a2458d7fdd12bddbe98eca
> src/main/java/org/apache/aurora/scheduler/TierManager.java 652afece17a7eb09b0ca68066707b1b8fbf024f0
> src/test/java/org/apache/aurora/scheduler/TierManagerTest.java eb0207114e1b93968cb65832f154a9cd3bc3232e
> src/test/resources/org/apache/aurora/scheduler/tiers-example.json PRE-CREATION
>
> Diff: https://reviews.apache.org/r/37825/diff/
>
>
> Testing
> -------
>
> ./gradlew -Pq build
> ./src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh
>
>
> Thanks,
>
> Maxim Khutornenko
>
>
Re: Review Request 37825: Adding minimal implementation of the
external tier config.
Posted by Maxim Khutornenko <ma...@apache.org>.
> On Aug. 29, 2015, 12:07 a.m., Zameer Manji wrote:
> > src/main/java/org/apache/aurora/scheduler/TierInfo.java, line 70
> > <https://reviews.apache.org/r/37825/diff/3/?file=1058906#file1058906line70>
> >
> > Would you mind setting an explicit default here?
Redundant, but sure, done.
- Maxim
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37825/#review96964
-----------------------------------------------------------
On Aug. 28, 2015, 11:46 p.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37825/
> -----------------------------------------------------------
>
> (Updated Aug. 28, 2015, 11:46 p.m.)
>
>
> Review request for Aurora, Bill Farner and Zameer Manji.
>
>
> Bugs: AURORA-1437
> https://issues.apache.org/jira/browse/AURORA-1437
>
>
> Repository: aurora
>
>
> Description
> -------
>
> The external config file is optional for now as tiers are not fully defined yet.
>
>
> Diffs
> -----
>
> examples/vagrant/upstart/aurora-scheduler-kerberos.conf 744b4a35c61e749734e222b3d4cbd296927665aa
> examples/vagrant/upstart/aurora-scheduler.conf 789a3a0315e8530880999432aa9b1e7d0f57d1ff
> src/jmh/java/org/apache/aurora/benchmark/Offers.java 26bad1cce987ef7f46368a5936cf056aeb2f63b1
> src/main/java/org/apache/aurora/scheduler/SchedulerModule.java d4bc6b9bb10c982fb6a2458d7fdd12bddbe98eca
> src/main/java/org/apache/aurora/scheduler/TierInfo.java 61bf30a7f11d2d4b9e49c58a6ed9ecd779d7e5ba
> src/main/java/org/apache/aurora/scheduler/TierManager.java 652afece17a7eb09b0ca68066707b1b8fbf024f0
> src/main/java/org/apache/aurora/scheduler/base/TaskTestUtil.java d7f3c60d383cf10afb0c0fcf4fe29972b183458c
> src/test/java/org/apache/aurora/scheduler/ResourceSlotTest.java 399f58de6196b97abd359ecef8131b63480d591a
> src/test/java/org/apache/aurora/scheduler/ResourcesTest.java eb1baedcb13c2f169d819d137f22cb8b88db149c
> src/test/java/org/apache/aurora/scheduler/SchedulerModuleTest.java PRE-CREATION
> src/test/java/org/apache/aurora/scheduler/TierManagerTest.java eb0207114e1b93968cb65832f154a9cd3bc3232e
> src/test/java/org/apache/aurora/scheduler/mesos/MesosTaskFactoryImplTest.java adcc7a823ecf30442016ecbdd655622d6aeba65e
> src/test/java/org/apache/aurora/scheduler/preemptor/PreemptionVictimFilterTest.java 819b51e4c314749dc48db25693503af7d1ed0c54
> src/test/java/org/apache/aurora/scheduler/state/TaskAssignerImplTest.java d8a524d98771ee68d7b4d423fb34e28101a04d27
> src/test/resources/org/apache/aurora/scheduler/tiers-example.json PRE-CREATION
>
> Diff: https://reviews.apache.org/r/37825/diff/
>
>
> Testing
> -------
>
> ./gradlew -Pq build
> ./src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh
>
>
> Thanks,
>
> Maxim Khutornenko
>
>
Re: Review Request 37825: Adding minimal implementation of the
external tier config.
Posted by Zameer Manji <zm...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37825/#review96964
-----------------------------------------------------------
Ship it!
src/main/java/org/apache/aurora/scheduler/TierInfo.java (line 67)
<https://reviews.apache.org/r/37825/#comment152666>
Would you mind setting an explicit default here?
- Zameer Manji
On Aug. 28, 2015, 4:46 p.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37825/
> -----------------------------------------------------------
>
> (Updated Aug. 28, 2015, 4:46 p.m.)
>
>
> Review request for Aurora, Bill Farner and Zameer Manji.
>
>
> Bugs: AURORA-1437
> https://issues.apache.org/jira/browse/AURORA-1437
>
>
> Repository: aurora
>
>
> Description
> -------
>
> The external config file is optional for now as tiers are not fully defined yet.
>
>
> Diffs
> -----
>
> examples/vagrant/upstart/aurora-scheduler-kerberos.conf 744b4a35c61e749734e222b3d4cbd296927665aa
> examples/vagrant/upstart/aurora-scheduler.conf 789a3a0315e8530880999432aa9b1e7d0f57d1ff
> src/jmh/java/org/apache/aurora/benchmark/Offers.java 26bad1cce987ef7f46368a5936cf056aeb2f63b1
> src/main/java/org/apache/aurora/scheduler/SchedulerModule.java d4bc6b9bb10c982fb6a2458d7fdd12bddbe98eca
> src/main/java/org/apache/aurora/scheduler/TierInfo.java 61bf30a7f11d2d4b9e49c58a6ed9ecd779d7e5ba
> src/main/java/org/apache/aurora/scheduler/TierManager.java 652afece17a7eb09b0ca68066707b1b8fbf024f0
> src/main/java/org/apache/aurora/scheduler/base/TaskTestUtil.java d7f3c60d383cf10afb0c0fcf4fe29972b183458c
> src/test/java/org/apache/aurora/scheduler/ResourceSlotTest.java 399f58de6196b97abd359ecef8131b63480d591a
> src/test/java/org/apache/aurora/scheduler/ResourcesTest.java eb1baedcb13c2f169d819d137f22cb8b88db149c
> src/test/java/org/apache/aurora/scheduler/SchedulerModuleTest.java PRE-CREATION
> src/test/java/org/apache/aurora/scheduler/TierManagerTest.java eb0207114e1b93968cb65832f154a9cd3bc3232e
> src/test/java/org/apache/aurora/scheduler/mesos/MesosTaskFactoryImplTest.java adcc7a823ecf30442016ecbdd655622d6aeba65e
> src/test/java/org/apache/aurora/scheduler/preemptor/PreemptionVictimFilterTest.java 819b51e4c314749dc48db25693503af7d1ed0c54
> src/test/java/org/apache/aurora/scheduler/state/TaskAssignerImplTest.java d8a524d98771ee68d7b4d423fb34e28101a04d27
> src/test/resources/org/apache/aurora/scheduler/tiers-example.json PRE-CREATION
>
> Diff: https://reviews.apache.org/r/37825/diff/
>
>
> Testing
> -------
>
> ./gradlew -Pq build
> ./src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh
>
>
> Thanks,
>
> Maxim Khutornenko
>
>
Re: Review Request 37825: Adding minimal implementation of the
external tier config.
Posted by Maxim Khutornenko <ma...@apache.org>.
> On Aug. 29, 2015, 5:41 a.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/SchedulerModule.java, line 73
> > <https://reviews.apache.org/r/37825/diff/4/?file=1058956#file1058956line73>
> >
> > What's preventing you from adding the annotation now?
This file's presence is optional until the feature graduates from beta. However, it appears null value is handled gracefully by the arg parser. Adding now.
> On Aug. 29, 2015, 5:41 a.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/SchedulerModule.java, line 74
> > <https://reviews.apache.org/r/37825/diff/4/?file=1058956#file1058956line74>
> >
> > How about just `tier_config`? Much more concise, and IMHO just as informative.
No preference, done.
> On Aug. 29, 2015, 5:41 a.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/SchedulerModule.java, line 75
> > <https://reviews.apache.org/r/37825/diff/4/?file=1058956#file1058956line75>
> >
> > Don't bother mentioning the format here, as the user will really have to use a doc to understand how to structure the file anyhow.
Dropped.
> On Aug. 29, 2015, 5:41 a.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/TierManager.java, line 45
> > <https://reviews.apache.org/r/37825/diff/4/?file=1058958#file1058958line45>
> >
> > This is repeated 3x. Please consider DRY and make this a constant in TierInfo.
Done.
> On Aug. 29, 2015, 5:41 a.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/TierManager.java, line 49
> > <https://reviews.apache.org/r/37825/diff/4/?file=1058958#file1058958line49>
> >
> > How about `TierConfig`? (this would also match the names being used elsewhere)
Sure, done.
> On Aug. 29, 2015, 5:41 a.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/TierInfo.java, line 23
> > <https://reviews.apache.org/r/37825/diff/4/?file=1058957#file1058957line23>
> >
> > With a few small changes, you can remove a lot of the excess code here without sacrificing much:
> >
> > ```
> > private TierInfo() {
> > this(false);
> > }
> >
> > public TierInfo(boolean revocable) {
> > this.revocable = revocable;
> > }
> > ```
> >
> > Then in SchedulerModule:
> > ```
> > ObjectMapper mapper = new ObjectMapper();
> > mapper.getDeserializationConfig().with(Feature.AUTO_DETECT_FIELDS);
> > return mapper.readValue(input, ConfigMap.class);
> > ```
> >
> > This saves you from having to introduce the Builder, so you can use TierInfo throughout, and you don't need annotations for jackson to work.
That's what I started with but changed later to avoid exposing constructors explicitly. Now looking at it again I agree it's more verbose for no good reason.
As for AUTO_DETECT_FIELDS it's on by default and only works for public fields: http://fasterxml.github.io/jackson-databind/javadoc/2.3.0/com/fasterxml/jackson/databind/MapperFeature.html#AUTO_DETECT_FIELDS
- Maxim
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37825/#review96973
-----------------------------------------------------------
On Aug. 29, 2015, 12:25 a.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37825/
> -----------------------------------------------------------
>
> (Updated Aug. 29, 2015, 12:25 a.m.)
>
>
> Review request for Aurora, Bill Farner and Zameer Manji.
>
>
> Bugs: AURORA-1437
> https://issues.apache.org/jira/browse/AURORA-1437
>
>
> Repository: aurora
>
>
> Description
> -------
>
> The external config file is optional for now as tiers are not fully defined yet.
>
>
> Diffs
> -----
>
> examples/vagrant/upstart/aurora-scheduler-kerberos.conf 744b4a35c61e749734e222b3d4cbd296927665aa
> examples/vagrant/upstart/aurora-scheduler.conf 789a3a0315e8530880999432aa9b1e7d0f57d1ff
> src/jmh/java/org/apache/aurora/benchmark/Offers.java 26bad1cce987ef7f46368a5936cf056aeb2f63b1
> src/main/java/org/apache/aurora/scheduler/SchedulerModule.java d4bc6b9bb10c982fb6a2458d7fdd12bddbe98eca
> src/main/java/org/apache/aurora/scheduler/TierInfo.java 61bf30a7f11d2d4b9e49c58a6ed9ecd779d7e5ba
> src/main/java/org/apache/aurora/scheduler/TierManager.java 652afece17a7eb09b0ca68066707b1b8fbf024f0
> src/main/java/org/apache/aurora/scheduler/base/TaskTestUtil.java d7f3c60d383cf10afb0c0fcf4fe29972b183458c
> src/test/java/org/apache/aurora/scheduler/ResourceSlotTest.java 399f58de6196b97abd359ecef8131b63480d591a
> src/test/java/org/apache/aurora/scheduler/ResourcesTest.java eb1baedcb13c2f169d819d137f22cb8b88db149c
> src/test/java/org/apache/aurora/scheduler/SchedulerModuleTest.java PRE-CREATION
> src/test/java/org/apache/aurora/scheduler/TierManagerTest.java eb0207114e1b93968cb65832f154a9cd3bc3232e
> src/test/java/org/apache/aurora/scheduler/mesos/MesosTaskFactoryImplTest.java adcc7a823ecf30442016ecbdd655622d6aeba65e
> src/test/java/org/apache/aurora/scheduler/preemptor/PreemptionVictimFilterTest.java 819b51e4c314749dc48db25693503af7d1ed0c54
> src/test/java/org/apache/aurora/scheduler/state/TaskAssignerImplTest.java d8a524d98771ee68d7b4d423fb34e28101a04d27
> src/test/resources/org/apache/aurora/scheduler/tiers-example.json PRE-CREATION
>
> Diff: https://reviews.apache.org/r/37825/diff/
>
>
> Testing
> -------
>
> ./gradlew -Pq build
> ./src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh
>
>
> Thanks,
>
> Maxim Khutornenko
>
>
Re: Review Request 37825: Adding minimal implementation of the
external tier config.
Posted by Bill Farner <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37825/#review96973
-----------------------------------------------------------
src/main/java/org/apache/aurora/scheduler/SchedulerModule.java (line 73)
<https://reviews.apache.org/r/37825/#comment152679>
What's preventing you from adding the annotation now?
src/main/java/org/apache/aurora/scheduler/SchedulerModule.java (line 74)
<https://reviews.apache.org/r/37825/#comment152678>
How about just `tier_config`? Much more concise, and IMHO just as informative.
src/main/java/org/apache/aurora/scheduler/SchedulerModule.java (line 75)
<https://reviews.apache.org/r/37825/#comment152682>
Don't bother mentioning the format here, as the user will really have to use a doc to understand how to structure the file anyhow.
src/main/java/org/apache/aurora/scheduler/TierInfo.java (line 21)
<https://reviews.apache.org/r/37825/#comment152681>
With a few small changes, you can remove a lot of the excess code here without sacrificing much:
```
private TierInfo() {
this(false);
}
public TierInfo(boolean revocable) {
this.revocable = revocable;
}
```
Then in SchedulerModule:
```
ObjectMapper mapper = new ObjectMapper();
mapper.getDeserializationConfig().with(Feature.AUTO_DETECT_FIELDS);
return mapper.readValue(input, ConfigMap.class);
```
This saves you from having to introduce the Builder, so you can use TierInfo throughout, and you don't need annotations for jackson to work.
src/main/java/org/apache/aurora/scheduler/TierManager.java (line 44)
<https://reviews.apache.org/r/37825/#comment152680>
This is repeated 3x. Please consider DRY and make this a constant in TierInfo.
src/main/java/org/apache/aurora/scheduler/TierManager.java (line 48)
<https://reviews.apache.org/r/37825/#comment152683>
How about `TierConfig`? (this would also match the names being used elsewhere)
- Bill Farner
On Aug. 28, 2015, 5:25 p.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37825/
> -----------------------------------------------------------
>
> (Updated Aug. 28, 2015, 5:25 p.m.)
>
>
> Review request for Aurora, Bill Farner and Zameer Manji.
>
>
> Bugs: AURORA-1437
> https://issues.apache.org/jira/browse/AURORA-1437
>
>
> Repository: aurora
>
>
> Description
> -------
>
> The external config file is optional for now as tiers are not fully defined yet.
>
>
> Diffs
> -----
>
> examples/vagrant/upstart/aurora-scheduler-kerberos.conf 744b4a35c61e749734e222b3d4cbd296927665aa
> examples/vagrant/upstart/aurora-scheduler.conf 789a3a0315e8530880999432aa9b1e7d0f57d1ff
> src/jmh/java/org/apache/aurora/benchmark/Offers.java 26bad1cce987ef7f46368a5936cf056aeb2f63b1
> src/main/java/org/apache/aurora/scheduler/SchedulerModule.java d4bc6b9bb10c982fb6a2458d7fdd12bddbe98eca
> src/main/java/org/apache/aurora/scheduler/TierInfo.java 61bf30a7f11d2d4b9e49c58a6ed9ecd779d7e5ba
> src/main/java/org/apache/aurora/scheduler/TierManager.java 652afece17a7eb09b0ca68066707b1b8fbf024f0
> src/main/java/org/apache/aurora/scheduler/base/TaskTestUtil.java d7f3c60d383cf10afb0c0fcf4fe29972b183458c
> src/test/java/org/apache/aurora/scheduler/ResourceSlotTest.java 399f58de6196b97abd359ecef8131b63480d591a
> src/test/java/org/apache/aurora/scheduler/ResourcesTest.java eb1baedcb13c2f169d819d137f22cb8b88db149c
> src/test/java/org/apache/aurora/scheduler/SchedulerModuleTest.java PRE-CREATION
> src/test/java/org/apache/aurora/scheduler/TierManagerTest.java eb0207114e1b93968cb65832f154a9cd3bc3232e
> src/test/java/org/apache/aurora/scheduler/mesos/MesosTaskFactoryImplTest.java adcc7a823ecf30442016ecbdd655622d6aeba65e
> src/test/java/org/apache/aurora/scheduler/preemptor/PreemptionVictimFilterTest.java 819b51e4c314749dc48db25693503af7d1ed0c54
> src/test/java/org/apache/aurora/scheduler/state/TaskAssignerImplTest.java d8a524d98771ee68d7b4d423fb34e28101a04d27
> src/test/resources/org/apache/aurora/scheduler/tiers-example.json PRE-CREATION
>
> Diff: https://reviews.apache.org/r/37825/diff/
>
>
> Testing
> -------
>
> ./gradlew -Pq build
> ./src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh
>
>
> Thanks,
>
> Maxim Khutornenko
>
>
Re: Review Request 37825: Adding minimal implementation of the
external tier config.
Posted by Maxim Khutornenko <ma...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37825/
-----------------------------------------------------------
(Updated Aug. 31, 2015, 10:17 p.m.)
Review request for Aurora, Bill Farner and Zameer Manji.
Changes
-------
Bill's feedback.
Bugs: AURORA-1437
https://issues.apache.org/jira/browse/AURORA-1437
Repository: aurora
Description
-------
The external config file is optional for now as tiers are not fully defined yet.
Diffs (updated)
-----
examples/vagrant/upstart/aurora-scheduler-kerberos.conf 744b4a35c61e749734e222b3d4cbd296927665aa
examples/vagrant/upstart/aurora-scheduler.conf 789a3a0315e8530880999432aa9b1e7d0f57d1ff
src/jmh/java/org/apache/aurora/benchmark/Offers.java 26bad1cce987ef7f46368a5936cf056aeb2f63b1
src/main/java/org/apache/aurora/scheduler/SchedulerModule.java d4bc6b9bb10c982fb6a2458d7fdd12bddbe98eca
src/main/java/org/apache/aurora/scheduler/TierInfo.java 72c2f9e1b6fe5bd603c3c29a1d2060be43e39db1
src/main/java/org/apache/aurora/scheduler/TierManager.java 652afece17a7eb09b0ca68066707b1b8fbf024f0
src/main/java/org/apache/aurora/scheduler/base/TaskTestUtil.java d7f3c60d383cf10afb0c0fcf4fe29972b183458c
src/test/java/org/apache/aurora/scheduler/ResourceSlotTest.java 399f58de6196b97abd359ecef8131b63480d591a
src/test/java/org/apache/aurora/scheduler/ResourcesTest.java eb1baedcb13c2f169d819d137f22cb8b88db149c
src/test/java/org/apache/aurora/scheduler/SchedulerModuleTest.java PRE-CREATION
src/test/java/org/apache/aurora/scheduler/TierManagerTest.java eb0207114e1b93968cb65832f154a9cd3bc3232e
src/test/java/org/apache/aurora/scheduler/mesos/MesosTaskFactoryImplTest.java adcc7a823ecf30442016ecbdd655622d6aeba65e
src/test/java/org/apache/aurora/scheduler/preemptor/PreemptionVictimFilterTest.java 819b51e4c314749dc48db25693503af7d1ed0c54
src/test/java/org/apache/aurora/scheduler/state/TaskAssignerImplTest.java d8a524d98771ee68d7b4d423fb34e28101a04d27
src/test/resources/org/apache/aurora/scheduler/tiers-example.json PRE-CREATION
Diff: https://reviews.apache.org/r/37825/diff/
Testing
-------
./gradlew -Pq build
./src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh
Thanks,
Maxim Khutornenko
Re: Review Request 37825: Adding minimal implementation of the
external tier config.
Posted by Maxim Khutornenko <ma...@apache.org>.
> On Aug. 30, 2015, 3:20 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/SchedulerModule.java, line 74
> > <https://reviews.apache.org/r/37825/diff/5/?file=1059384#file1059384line74>
> >
> > You might consider killing the TODO. There probably isn't a great way to link to the correct documentation, as it would really need to be associated with the version.
Done.
> On Aug. 30, 2015, 3:20 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/TierInfo.java, line 24
> > <https://reviews.apache.org/r/37825/diff/5/?file=1059385#file1059385line24>
> >
> > Totally nit-picking - `TierInfo.DEFAULT_TIER` is a bit redundant, as the containing class and the field type already contain 'tier'. Consider `TierInfo.DEFAULT`, and skip the static import where it's used to keep the context.
Done.
> On Aug. 30, 2015, 3:20 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/TierManager.java, line 55
> > <https://reviews.apache.org/r/37825/diff/5/?file=1059386#file1059386line55>
> >
> > You could avoid the supplier/memoization and instead use a traditional constructor with annotations:
> >
> > ```
> > static final TierConfig EMPTY = new TierConfig(ImmutableMap.of());
> >
> > private final Map<String, TierInfo> tiers;
> >
> > @JsonCreator
> > TierConfig(@JsonProperty("tiers") Map<String, TierInfo> tiers) {
> > this.tiers = ImmutableMap.copyOf(tiers);
> > }
> > ```
> >
> > I like this since the 'JSON-ness' is completely isolated to the constructor.
Happy to accept. Frankly, jackson json tuning is such a PITA!
> On Aug. 30, 2015, 3:20 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/TierManager.java, line 50
> > <https://reviews.apache.org/r/37825/diff/5/?file=1059386#file1059386line50>
> >
> > You could avoid the annotation with this mapper setting:
> > ```
> > ObjectMapper mapper = new ObjectMapper();
> > mapper.setVisibilityChecker(
> > mapper.getVisibilityChecker().withFieldVisibility(Visibility.ANY));
> > ```
> >
> > However, the advice below would also remove it and is IMHO a slightly better approach.
Took advice below.
- Maxim
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37825/#review97027
-----------------------------------------------------------
On Aug. 29, 2015, 6:34 p.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37825/
> -----------------------------------------------------------
>
> (Updated Aug. 29, 2015, 6:34 p.m.)
>
>
> Review request for Aurora, Bill Farner and Zameer Manji.
>
>
> Bugs: AURORA-1437
> https://issues.apache.org/jira/browse/AURORA-1437
>
>
> Repository: aurora
>
>
> Description
> -------
>
> The external config file is optional for now as tiers are not fully defined yet.
>
>
> Diffs
> -----
>
> examples/vagrant/upstart/aurora-scheduler-kerberos.conf 744b4a35c61e749734e222b3d4cbd296927665aa
> examples/vagrant/upstart/aurora-scheduler.conf 789a3a0315e8530880999432aa9b1e7d0f57d1ff
> src/jmh/java/org/apache/aurora/benchmark/Offers.java 26bad1cce987ef7f46368a5936cf056aeb2f63b1
> src/main/java/org/apache/aurora/scheduler/SchedulerModule.java d4bc6b9bb10c982fb6a2458d7fdd12bddbe98eca
> src/main/java/org/apache/aurora/scheduler/TierInfo.java 61bf30a7f11d2d4b9e49c58a6ed9ecd779d7e5ba
> src/main/java/org/apache/aurora/scheduler/TierManager.java 652afece17a7eb09b0ca68066707b1b8fbf024f0
> src/main/java/org/apache/aurora/scheduler/base/TaskTestUtil.java d7f3c60d383cf10afb0c0fcf4fe29972b183458c
> src/test/java/org/apache/aurora/scheduler/ResourceSlotTest.java 399f58de6196b97abd359ecef8131b63480d591a
> src/test/java/org/apache/aurora/scheduler/ResourcesTest.java eb1baedcb13c2f169d819d137f22cb8b88db149c
> src/test/java/org/apache/aurora/scheduler/SchedulerModuleTest.java PRE-CREATION
> src/test/java/org/apache/aurora/scheduler/TierManagerTest.java eb0207114e1b93968cb65832f154a9cd3bc3232e
> src/test/java/org/apache/aurora/scheduler/mesos/MesosTaskFactoryImplTest.java adcc7a823ecf30442016ecbdd655622d6aeba65e
> src/test/java/org/apache/aurora/scheduler/preemptor/PreemptionVictimFilterTest.java 819b51e4c314749dc48db25693503af7d1ed0c54
> src/test/java/org/apache/aurora/scheduler/state/TaskAssignerImplTest.java d8a524d98771ee68d7b4d423fb34e28101a04d27
> src/test/resources/org/apache/aurora/scheduler/tiers-example.json PRE-CREATION
>
> Diff: https://reviews.apache.org/r/37825/diff/
>
>
> Testing
> -------
>
> ./gradlew -Pq build
> ./src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh
>
>
> Thanks,
>
> Maxim Khutornenko
>
>
Re: Review Request 37825: Adding minimal implementation of the
external tier config.
Posted by Bill Farner <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37825/#review97027
-----------------------------------------------------------
Ship it!
src/main/java/org/apache/aurora/scheduler/SchedulerModule.java (line 74)
<https://reviews.apache.org/r/37825/#comment152739>
You might consider killing the TODO. There probably isn't a great way to link to the correct documentation, as it would really need to be associated with the version.
src/main/java/org/apache/aurora/scheduler/TierInfo.java (line 22)
<https://reviews.apache.org/r/37825/#comment152742>
Totally nit-picking - `TierInfo.DEFAULT_TIER` is a bit redundant, as the containing class and the field type already contain 'tier'. Consider `TierInfo.DEFAULT`, and skip the static import where it's used to keep the context.
src/main/java/org/apache/aurora/scheduler/TierManager.java (line 49)
<https://reviews.apache.org/r/37825/#comment152741>
You could avoid the annotation with this mapper setting:
```
ObjectMapper mapper = new ObjectMapper();
mapper.setVisibilityChecker(
mapper.getVisibilityChecker().withFieldVisibility(Visibility.ANY));
```
However, the advice below would also remove it and is IMHO a slightly better approach.
src/main/java/org/apache/aurora/scheduler/TierManager.java (line 54)
<https://reviews.apache.org/r/37825/#comment152740>
You could avoid the supplier/memoization and instead use a traditional constructor with annotations:
```
static final TierConfig EMPTY = new TierConfig(ImmutableMap.of());
private final Map<String, TierInfo> tiers;
@JsonCreator
TierConfig(@JsonProperty("tiers") Map<String, TierInfo> tiers) {
this.tiers = ImmutableMap.copyOf(tiers);
}
```
I like this since the 'JSON-ness' is completely isolated to the constructor.
- Bill Farner
On Aug. 29, 2015, 11:34 a.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37825/
> -----------------------------------------------------------
>
> (Updated Aug. 29, 2015, 11:34 a.m.)
>
>
> Review request for Aurora, Bill Farner and Zameer Manji.
>
>
> Bugs: AURORA-1437
> https://issues.apache.org/jira/browse/AURORA-1437
>
>
> Repository: aurora
>
>
> Description
> -------
>
> The external config file is optional for now as tiers are not fully defined yet.
>
>
> Diffs
> -----
>
> examples/vagrant/upstart/aurora-scheduler-kerberos.conf 744b4a35c61e749734e222b3d4cbd296927665aa
> examples/vagrant/upstart/aurora-scheduler.conf 789a3a0315e8530880999432aa9b1e7d0f57d1ff
> src/jmh/java/org/apache/aurora/benchmark/Offers.java 26bad1cce987ef7f46368a5936cf056aeb2f63b1
> src/main/java/org/apache/aurora/scheduler/SchedulerModule.java d4bc6b9bb10c982fb6a2458d7fdd12bddbe98eca
> src/main/java/org/apache/aurora/scheduler/TierInfo.java 61bf30a7f11d2d4b9e49c58a6ed9ecd779d7e5ba
> src/main/java/org/apache/aurora/scheduler/TierManager.java 652afece17a7eb09b0ca68066707b1b8fbf024f0
> src/main/java/org/apache/aurora/scheduler/base/TaskTestUtil.java d7f3c60d383cf10afb0c0fcf4fe29972b183458c
> src/test/java/org/apache/aurora/scheduler/ResourceSlotTest.java 399f58de6196b97abd359ecef8131b63480d591a
> src/test/java/org/apache/aurora/scheduler/ResourcesTest.java eb1baedcb13c2f169d819d137f22cb8b88db149c
> src/test/java/org/apache/aurora/scheduler/SchedulerModuleTest.java PRE-CREATION
> src/test/java/org/apache/aurora/scheduler/TierManagerTest.java eb0207114e1b93968cb65832f154a9cd3bc3232e
> src/test/java/org/apache/aurora/scheduler/mesos/MesosTaskFactoryImplTest.java adcc7a823ecf30442016ecbdd655622d6aeba65e
> src/test/java/org/apache/aurora/scheduler/preemptor/PreemptionVictimFilterTest.java 819b51e4c314749dc48db25693503af7d1ed0c54
> src/test/java/org/apache/aurora/scheduler/state/TaskAssignerImplTest.java d8a524d98771ee68d7b4d423fb34e28101a04d27
> src/test/resources/org/apache/aurora/scheduler/tiers-example.json PRE-CREATION
>
> Diff: https://reviews.apache.org/r/37825/diff/
>
>
> Testing
> -------
>
> ./gradlew -Pq build
> ./src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh
>
>
> Thanks,
>
> Maxim Khutornenko
>
>
Re: Review Request 37825: Adding minimal implementation of the
external tier config.
Posted by Aurora ReviewBot <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37825/#review96997
-----------------------------------------------------------
Ship it!
Master (782f883) 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 Aug. 29, 2015, 6:34 p.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37825/
> -----------------------------------------------------------
>
> (Updated Aug. 29, 2015, 6:34 p.m.)
>
>
> Review request for Aurora, Bill Farner and Zameer Manji.
>
>
> Bugs: AURORA-1437
> https://issues.apache.org/jira/browse/AURORA-1437
>
>
> Repository: aurora
>
>
> Description
> -------
>
> The external config file is optional for now as tiers are not fully defined yet.
>
>
> Diffs
> -----
>
> examples/vagrant/upstart/aurora-scheduler-kerberos.conf 744b4a35c61e749734e222b3d4cbd296927665aa
> examples/vagrant/upstart/aurora-scheduler.conf 789a3a0315e8530880999432aa9b1e7d0f57d1ff
> src/jmh/java/org/apache/aurora/benchmark/Offers.java 26bad1cce987ef7f46368a5936cf056aeb2f63b1
> src/main/java/org/apache/aurora/scheduler/SchedulerModule.java d4bc6b9bb10c982fb6a2458d7fdd12bddbe98eca
> src/main/java/org/apache/aurora/scheduler/TierInfo.java 61bf30a7f11d2d4b9e49c58a6ed9ecd779d7e5ba
> src/main/java/org/apache/aurora/scheduler/TierManager.java 652afece17a7eb09b0ca68066707b1b8fbf024f0
> src/main/java/org/apache/aurora/scheduler/base/TaskTestUtil.java d7f3c60d383cf10afb0c0fcf4fe29972b183458c
> src/test/java/org/apache/aurora/scheduler/ResourceSlotTest.java 399f58de6196b97abd359ecef8131b63480d591a
> src/test/java/org/apache/aurora/scheduler/ResourcesTest.java eb1baedcb13c2f169d819d137f22cb8b88db149c
> src/test/java/org/apache/aurora/scheduler/SchedulerModuleTest.java PRE-CREATION
> src/test/java/org/apache/aurora/scheduler/TierManagerTest.java eb0207114e1b93968cb65832f154a9cd3bc3232e
> src/test/java/org/apache/aurora/scheduler/mesos/MesosTaskFactoryImplTest.java adcc7a823ecf30442016ecbdd655622d6aeba65e
> src/test/java/org/apache/aurora/scheduler/preemptor/PreemptionVictimFilterTest.java 819b51e4c314749dc48db25693503af7d1ed0c54
> src/test/java/org/apache/aurora/scheduler/state/TaskAssignerImplTest.java d8a524d98771ee68d7b4d423fb34e28101a04d27
> src/test/resources/org/apache/aurora/scheduler/tiers-example.json PRE-CREATION
>
> Diff: https://reviews.apache.org/r/37825/diff/
>
>
> Testing
> -------
>
> ./gradlew -Pq build
> ./src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh
>
>
> Thanks,
>
> Maxim Khutornenko
>
>
Re: Review Request 37825: Adding minimal implementation of the
external tier config.
Posted by Maxim Khutornenko <ma...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37825/
-----------------------------------------------------------
(Updated Aug. 29, 2015, 6:34 p.m.)
Review request for Aurora, Bill Farner and Zameer Manji.
Changes
-------
Bill's comments.
Bugs: AURORA-1437
https://issues.apache.org/jira/browse/AURORA-1437
Repository: aurora
Description
-------
The external config file is optional for now as tiers are not fully defined yet.
Diffs (updated)
-----
examples/vagrant/upstart/aurora-scheduler-kerberos.conf 744b4a35c61e749734e222b3d4cbd296927665aa
examples/vagrant/upstart/aurora-scheduler.conf 789a3a0315e8530880999432aa9b1e7d0f57d1ff
src/jmh/java/org/apache/aurora/benchmark/Offers.java 26bad1cce987ef7f46368a5936cf056aeb2f63b1
src/main/java/org/apache/aurora/scheduler/SchedulerModule.java d4bc6b9bb10c982fb6a2458d7fdd12bddbe98eca
src/main/java/org/apache/aurora/scheduler/TierInfo.java 61bf30a7f11d2d4b9e49c58a6ed9ecd779d7e5ba
src/main/java/org/apache/aurora/scheduler/TierManager.java 652afece17a7eb09b0ca68066707b1b8fbf024f0
src/main/java/org/apache/aurora/scheduler/base/TaskTestUtil.java d7f3c60d383cf10afb0c0fcf4fe29972b183458c
src/test/java/org/apache/aurora/scheduler/ResourceSlotTest.java 399f58de6196b97abd359ecef8131b63480d591a
src/test/java/org/apache/aurora/scheduler/ResourcesTest.java eb1baedcb13c2f169d819d137f22cb8b88db149c
src/test/java/org/apache/aurora/scheduler/SchedulerModuleTest.java PRE-CREATION
src/test/java/org/apache/aurora/scheduler/TierManagerTest.java eb0207114e1b93968cb65832f154a9cd3bc3232e
src/test/java/org/apache/aurora/scheduler/mesos/MesosTaskFactoryImplTest.java adcc7a823ecf30442016ecbdd655622d6aeba65e
src/test/java/org/apache/aurora/scheduler/preemptor/PreemptionVictimFilterTest.java 819b51e4c314749dc48db25693503af7d1ed0c54
src/test/java/org/apache/aurora/scheduler/state/TaskAssignerImplTest.java d8a524d98771ee68d7b4d423fb34e28101a04d27
src/test/resources/org/apache/aurora/scheduler/tiers-example.json PRE-CREATION
Diff: https://reviews.apache.org/r/37825/diff/
Testing
-------
./gradlew -Pq build
./src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh
Thanks,
Maxim Khutornenko
Re: Review Request 37825: Adding minimal implementation of the
external tier config.
Posted by Aurora ReviewBot <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37825/#review96967
-----------------------------------------------------------
Ship it!
Master (356eeac) 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 Aug. 29, 2015, 12:25 a.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37825/
> -----------------------------------------------------------
>
> (Updated Aug. 29, 2015, 12:25 a.m.)
>
>
> Review request for Aurora, Bill Farner and Zameer Manji.
>
>
> Bugs: AURORA-1437
> https://issues.apache.org/jira/browse/AURORA-1437
>
>
> Repository: aurora
>
>
> Description
> -------
>
> The external config file is optional for now as tiers are not fully defined yet.
>
>
> Diffs
> -----
>
> examples/vagrant/upstart/aurora-scheduler-kerberos.conf 744b4a35c61e749734e222b3d4cbd296927665aa
> examples/vagrant/upstart/aurora-scheduler.conf 789a3a0315e8530880999432aa9b1e7d0f57d1ff
> src/jmh/java/org/apache/aurora/benchmark/Offers.java 26bad1cce987ef7f46368a5936cf056aeb2f63b1
> src/main/java/org/apache/aurora/scheduler/SchedulerModule.java d4bc6b9bb10c982fb6a2458d7fdd12bddbe98eca
> src/main/java/org/apache/aurora/scheduler/TierInfo.java 61bf30a7f11d2d4b9e49c58a6ed9ecd779d7e5ba
> src/main/java/org/apache/aurora/scheduler/TierManager.java 652afece17a7eb09b0ca68066707b1b8fbf024f0
> src/main/java/org/apache/aurora/scheduler/base/TaskTestUtil.java d7f3c60d383cf10afb0c0fcf4fe29972b183458c
> src/test/java/org/apache/aurora/scheduler/ResourceSlotTest.java 399f58de6196b97abd359ecef8131b63480d591a
> src/test/java/org/apache/aurora/scheduler/ResourcesTest.java eb1baedcb13c2f169d819d137f22cb8b88db149c
> src/test/java/org/apache/aurora/scheduler/SchedulerModuleTest.java PRE-CREATION
> src/test/java/org/apache/aurora/scheduler/TierManagerTest.java eb0207114e1b93968cb65832f154a9cd3bc3232e
> src/test/java/org/apache/aurora/scheduler/mesos/MesosTaskFactoryImplTest.java adcc7a823ecf30442016ecbdd655622d6aeba65e
> src/test/java/org/apache/aurora/scheduler/preemptor/PreemptionVictimFilterTest.java 819b51e4c314749dc48db25693503af7d1ed0c54
> src/test/java/org/apache/aurora/scheduler/state/TaskAssignerImplTest.java d8a524d98771ee68d7b4d423fb34e28101a04d27
> src/test/resources/org/apache/aurora/scheduler/tiers-example.json PRE-CREATION
>
> Diff: https://reviews.apache.org/r/37825/diff/
>
>
> Testing
> -------
>
> ./gradlew -Pq build
> ./src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh
>
>
> Thanks,
>
> Maxim Khutornenko
>
>
Re: Review Request 37825: Adding minimal implementation of the
external tier config.
Posted by Maxim Khutornenko <ma...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37825/
-----------------------------------------------------------
(Updated Aug. 29, 2015, 12:25 a.m.)
Review request for Aurora, Bill Farner and Zameer Manji.
Changes
-------
Default for revocable.
Bugs: AURORA-1437
https://issues.apache.org/jira/browse/AURORA-1437
Repository: aurora
Description
-------
The external config file is optional for now as tiers are not fully defined yet.
Diffs (updated)
-----
examples/vagrant/upstart/aurora-scheduler-kerberos.conf 744b4a35c61e749734e222b3d4cbd296927665aa
examples/vagrant/upstart/aurora-scheduler.conf 789a3a0315e8530880999432aa9b1e7d0f57d1ff
src/jmh/java/org/apache/aurora/benchmark/Offers.java 26bad1cce987ef7f46368a5936cf056aeb2f63b1
src/main/java/org/apache/aurora/scheduler/SchedulerModule.java d4bc6b9bb10c982fb6a2458d7fdd12bddbe98eca
src/main/java/org/apache/aurora/scheduler/TierInfo.java 61bf30a7f11d2d4b9e49c58a6ed9ecd779d7e5ba
src/main/java/org/apache/aurora/scheduler/TierManager.java 652afece17a7eb09b0ca68066707b1b8fbf024f0
src/main/java/org/apache/aurora/scheduler/base/TaskTestUtil.java d7f3c60d383cf10afb0c0fcf4fe29972b183458c
src/test/java/org/apache/aurora/scheduler/ResourceSlotTest.java 399f58de6196b97abd359ecef8131b63480d591a
src/test/java/org/apache/aurora/scheduler/ResourcesTest.java eb1baedcb13c2f169d819d137f22cb8b88db149c
src/test/java/org/apache/aurora/scheduler/SchedulerModuleTest.java PRE-CREATION
src/test/java/org/apache/aurora/scheduler/TierManagerTest.java eb0207114e1b93968cb65832f154a9cd3bc3232e
src/test/java/org/apache/aurora/scheduler/mesos/MesosTaskFactoryImplTest.java adcc7a823ecf30442016ecbdd655622d6aeba65e
src/test/java/org/apache/aurora/scheduler/preemptor/PreemptionVictimFilterTest.java 819b51e4c314749dc48db25693503af7d1ed0c54
src/test/java/org/apache/aurora/scheduler/state/TaskAssignerImplTest.java d8a524d98771ee68d7b4d423fb34e28101a04d27
src/test/resources/org/apache/aurora/scheduler/tiers-example.json PRE-CREATION
Diff: https://reviews.apache.org/r/37825/diff/
Testing
-------
./gradlew -Pq build
./src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh
Thanks,
Maxim Khutornenko
Re: Review Request 37825: Adding minimal implementation of the
external tier config.
Posted by Aurora ReviewBot <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37825/#review96963
-----------------------------------------------------------
Ship it!
Master (356eeac) 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 Aug. 28, 2015, 11:46 p.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37825/
> -----------------------------------------------------------
>
> (Updated Aug. 28, 2015, 11:46 p.m.)
>
>
> Review request for Aurora, Bill Farner and Zameer Manji.
>
>
> Bugs: AURORA-1437
> https://issues.apache.org/jira/browse/AURORA-1437
>
>
> Repository: aurora
>
>
> Description
> -------
>
> The external config file is optional for now as tiers are not fully defined yet.
>
>
> Diffs
> -----
>
> examples/vagrant/upstart/aurora-scheduler-kerberos.conf 744b4a35c61e749734e222b3d4cbd296927665aa
> examples/vagrant/upstart/aurora-scheduler.conf 789a3a0315e8530880999432aa9b1e7d0f57d1ff
> src/jmh/java/org/apache/aurora/benchmark/Offers.java 26bad1cce987ef7f46368a5936cf056aeb2f63b1
> src/main/java/org/apache/aurora/scheduler/SchedulerModule.java d4bc6b9bb10c982fb6a2458d7fdd12bddbe98eca
> src/main/java/org/apache/aurora/scheduler/TierInfo.java 61bf30a7f11d2d4b9e49c58a6ed9ecd779d7e5ba
> src/main/java/org/apache/aurora/scheduler/TierManager.java 652afece17a7eb09b0ca68066707b1b8fbf024f0
> src/main/java/org/apache/aurora/scheduler/base/TaskTestUtil.java d7f3c60d383cf10afb0c0fcf4fe29972b183458c
> src/test/java/org/apache/aurora/scheduler/ResourceSlotTest.java 399f58de6196b97abd359ecef8131b63480d591a
> src/test/java/org/apache/aurora/scheduler/ResourcesTest.java eb1baedcb13c2f169d819d137f22cb8b88db149c
> src/test/java/org/apache/aurora/scheduler/SchedulerModuleTest.java PRE-CREATION
> src/test/java/org/apache/aurora/scheduler/TierManagerTest.java eb0207114e1b93968cb65832f154a9cd3bc3232e
> src/test/java/org/apache/aurora/scheduler/mesos/MesosTaskFactoryImplTest.java adcc7a823ecf30442016ecbdd655622d6aeba65e
> src/test/java/org/apache/aurora/scheduler/preemptor/PreemptionVictimFilterTest.java 819b51e4c314749dc48db25693503af7d1ed0c54
> src/test/java/org/apache/aurora/scheduler/state/TaskAssignerImplTest.java d8a524d98771ee68d7b4d423fb34e28101a04d27
> src/test/resources/org/apache/aurora/scheduler/tiers-example.json PRE-CREATION
>
> Diff: https://reviews.apache.org/r/37825/diff/
>
>
> Testing
> -------
>
> ./gradlew -Pq build
> ./src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh
>
>
> Thanks,
>
> Maxim Khutornenko
>
>
Re: Review Request 37825: Adding minimal implementation of the
external tier config.
Posted by Maxim Khutornenko <ma...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37825/
-----------------------------------------------------------
(Updated Aug. 28, 2015, 11:46 p.m.)
Review request for Aurora, Bill Farner and Zameer Manji.
Changes
-------
Using Jackson json parsing to fail on unknown fields.
Bugs: AURORA-1437
https://issues.apache.org/jira/browse/AURORA-1437
Repository: aurora
Description
-------
The external config file is optional for now as tiers are not fully defined yet.
Diffs (updated)
-----
examples/vagrant/upstart/aurora-scheduler-kerberos.conf 744b4a35c61e749734e222b3d4cbd296927665aa
examples/vagrant/upstart/aurora-scheduler.conf 789a3a0315e8530880999432aa9b1e7d0f57d1ff
src/jmh/java/org/apache/aurora/benchmark/Offers.java 26bad1cce987ef7f46368a5936cf056aeb2f63b1
src/main/java/org/apache/aurora/scheduler/SchedulerModule.java d4bc6b9bb10c982fb6a2458d7fdd12bddbe98eca
src/main/java/org/apache/aurora/scheduler/TierInfo.java 61bf30a7f11d2d4b9e49c58a6ed9ecd779d7e5ba
src/main/java/org/apache/aurora/scheduler/TierManager.java 652afece17a7eb09b0ca68066707b1b8fbf024f0
src/main/java/org/apache/aurora/scheduler/base/TaskTestUtil.java d7f3c60d383cf10afb0c0fcf4fe29972b183458c
src/test/java/org/apache/aurora/scheduler/ResourceSlotTest.java 399f58de6196b97abd359ecef8131b63480d591a
src/test/java/org/apache/aurora/scheduler/ResourcesTest.java eb1baedcb13c2f169d819d137f22cb8b88db149c
src/test/java/org/apache/aurora/scheduler/SchedulerModuleTest.java PRE-CREATION
src/test/java/org/apache/aurora/scheduler/TierManagerTest.java eb0207114e1b93968cb65832f154a9cd3bc3232e
src/test/java/org/apache/aurora/scheduler/mesos/MesosTaskFactoryImplTest.java adcc7a823ecf30442016ecbdd655622d6aeba65e
src/test/java/org/apache/aurora/scheduler/preemptor/PreemptionVictimFilterTest.java 819b51e4c314749dc48db25693503af7d1ed0c54
src/test/java/org/apache/aurora/scheduler/state/TaskAssignerImplTest.java d8a524d98771ee68d7b4d423fb34e28101a04d27
src/test/resources/org/apache/aurora/scheduler/tiers-example.json PRE-CREATION
Diff: https://reviews.apache.org/r/37825/diff/
Testing
-------
./gradlew -Pq build
./src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh
Thanks,
Maxim Khutornenko
Re: Review Request 37825: Adding minimal implementation of the
external tier config.
Posted by Aurora ReviewBot <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37825/#review96771
-----------------------------------------------------------
Ship it!
Master (06ddaad) 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 Aug. 27, 2015, 9:17 p.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37825/
> -----------------------------------------------------------
>
> (Updated Aug. 27, 2015, 9:17 p.m.)
>
>
> Review request for Aurora, Bill Farner and Zameer Manji.
>
>
> Bugs: AURORA-1437
> https://issues.apache.org/jira/browse/AURORA-1437
>
>
> Repository: aurora
>
>
> Description
> -------
>
> The external config file is optional for now as tiers are not fully defined yet.
>
>
> Diffs
> -----
>
> examples/vagrant/upstart/aurora-scheduler-kerberos.conf 744b4a35c61e749734e222b3d4cbd296927665aa
> examples/vagrant/upstart/aurora-scheduler.conf 789a3a0315e8530880999432aa9b1e7d0f57d1ff
> src/main/java/org/apache/aurora/scheduler/SchedulerModule.java d4bc6b9bb10c982fb6a2458d7fdd12bddbe98eca
> src/main/java/org/apache/aurora/scheduler/TierManager.java 652afece17a7eb09b0ca68066707b1b8fbf024f0
> src/test/java/org/apache/aurora/scheduler/SchedulerModuleTest.java PRE-CREATION
> src/test/java/org/apache/aurora/scheduler/TierManagerTest.java eb0207114e1b93968cb65832f154a9cd3bc3232e
> src/test/resources/org/apache/aurora/scheduler/tiers-example.json PRE-CREATION
>
> Diff: https://reviews.apache.org/r/37825/diff/
>
>
> Testing
> -------
>
> ./gradlew -Pq build
> ./src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh
>
>
> Thanks,
>
> Maxim Khutornenko
>
>
Re: Review Request 37825: Adding minimal implementation of the
external tier config.
Posted by Maxim Khutornenko <ma...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37825/
-----------------------------------------------------------
(Updated Aug. 27, 2015, 9:17 p.m.)
Review request for Aurora, Bill Farner and Zameer Manji.
Changes
-------
Bill's and Zameer's comments.
Bugs: AURORA-1437
https://issues.apache.org/jira/browse/AURORA-1437
Repository: aurora
Description
-------
The external config file is optional for now as tiers are not fully defined yet.
Diffs (updated)
-----
examples/vagrant/upstart/aurora-scheduler-kerberos.conf 744b4a35c61e749734e222b3d4cbd296927665aa
examples/vagrant/upstart/aurora-scheduler.conf 789a3a0315e8530880999432aa9b1e7d0f57d1ff
src/main/java/org/apache/aurora/scheduler/SchedulerModule.java d4bc6b9bb10c982fb6a2458d7fdd12bddbe98eca
src/main/java/org/apache/aurora/scheduler/TierManager.java 652afece17a7eb09b0ca68066707b1b8fbf024f0
src/test/java/org/apache/aurora/scheduler/SchedulerModuleTest.java PRE-CREATION
src/test/java/org/apache/aurora/scheduler/TierManagerTest.java eb0207114e1b93968cb65832f154a9cd3bc3232e
src/test/resources/org/apache/aurora/scheduler/tiers-example.json PRE-CREATION
Diff: https://reviews.apache.org/r/37825/diff/
Testing
-------
./gradlew -Pq build
./src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh
Thanks,
Maxim Khutornenko
Re: Review Request 37825: Adding minimal implementation of the
external tier config.
Posted by Aurora ReviewBot <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37825/#review96730
-----------------------------------------------------------
Ship it!
Master (06ddaad) 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 Aug. 27, 2015, 1:07 a.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37825/
> -----------------------------------------------------------
>
> (Updated Aug. 27, 2015, 1:07 a.m.)
>
>
> Review request for Aurora, Bill Farner and Zameer Manji.
>
>
> Bugs: AURORA-1437
> https://issues.apache.org/jira/browse/AURORA-1437
>
>
> Repository: aurora
>
>
> Description
> -------
>
> The external config file is optional for now as tiers are not fully defined yet.
>
>
> Diffs
> -----
>
> examples/vagrant/upstart/aurora-scheduler-kerberos.conf 744b4a35c61e749734e222b3d4cbd296927665aa
> examples/vagrant/upstart/aurora-scheduler.conf 789a3a0315e8530880999432aa9b1e7d0f57d1ff
> src/main/java/org/apache/aurora/scheduler/SchedulerModule.java d4bc6b9bb10c982fb6a2458d7fdd12bddbe98eca
> src/main/java/org/apache/aurora/scheduler/TierManager.java 652afece17a7eb09b0ca68066707b1b8fbf024f0
> src/test/java/org/apache/aurora/scheduler/TierManagerTest.java eb0207114e1b93968cb65832f154a9cd3bc3232e
> src/test/resources/org/apache/aurora/scheduler/tiers-example.json PRE-CREATION
>
> Diff: https://reviews.apache.org/r/37825/diff/
>
>
> Testing
> -------
>
> ./gradlew -Pq build
> ./src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh
>
>
> Thanks,
>
> Maxim Khutornenko
>
>
Re: Review Request 37825: Adding minimal implementation of the
external tier config.
Posted by Bill Farner <wf...@apache.org>.
> On Aug. 27, 2015, 10:54 a.m., Zameer Manji wrote:
> > src/test/java/org/apache/aurora/scheduler/TierManagerTest.java, line 26
> > <https://reviews.apache.org/r/37825/diff/1/?file=1055552#file1055552line26>
> >
> > Please add a test for the config for having keys that are not expected. I would expect the scheduler to reject such files because they are malformed.
>
> Maxim Khutornenko wrote:
> Not sure what you mean by malformed. Are you suggesting adding extra keys or purposefully corrupt json file? The former is irrelevant as unknown keys are ignored on casting to the type and the latter just throws unhandled parsing exception (exactly what we want/expect).
>
> Zameer Manji wrote:
> I think if there are extra keys we should reject the file even if the keys are irrelevant. I think operators would want to know if the configuration contains typos or other errors.
>
> Maxim Khutornenko wrote:
> That's not unreasonable. I'll defer it to AURORA-1443 though as the validation will have to be done on the TierInfo side and would make much more sense when we get to multiple tiers.
I suggest you address unknown field rejection now if it can be done with a small amount of effort (which i think it can). Looks like gson is permissive by design, but jackson by default has the behavior you want. IIRC we already depend on jackson.
```
new ObjectMapper().readValue(content, ConfigMap.class);
```
- Bill
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37825/#review96714
-----------------------------------------------------------
On Aug. 27, 2015, 2:17 p.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37825/
> -----------------------------------------------------------
>
> (Updated Aug. 27, 2015, 2:17 p.m.)
>
>
> Review request for Aurora, Bill Farner and Zameer Manji.
>
>
> Bugs: AURORA-1437
> https://issues.apache.org/jira/browse/AURORA-1437
>
>
> Repository: aurora
>
>
> Description
> -------
>
> The external config file is optional for now as tiers are not fully defined yet.
>
>
> Diffs
> -----
>
> examples/vagrant/upstart/aurora-scheduler-kerberos.conf 744b4a35c61e749734e222b3d4cbd296927665aa
> examples/vagrant/upstart/aurora-scheduler.conf 789a3a0315e8530880999432aa9b1e7d0f57d1ff
> src/main/java/org/apache/aurora/scheduler/SchedulerModule.java d4bc6b9bb10c982fb6a2458d7fdd12bddbe98eca
> src/main/java/org/apache/aurora/scheduler/TierManager.java 652afece17a7eb09b0ca68066707b1b8fbf024f0
> src/test/java/org/apache/aurora/scheduler/SchedulerModuleTest.java PRE-CREATION
> src/test/java/org/apache/aurora/scheduler/TierManagerTest.java eb0207114e1b93968cb65832f154a9cd3bc3232e
> src/test/resources/org/apache/aurora/scheduler/tiers-example.json PRE-CREATION
>
> Diff: https://reviews.apache.org/r/37825/diff/
>
>
> Testing
> -------
>
> ./gradlew -Pq build
> ./src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh
>
>
> Thanks,
>
> Maxim Khutornenko
>
>
Re: Review Request 37825: Adding minimal implementation of the
external tier config.
Posted by Zameer Manji <zm...@apache.org>.
> On Aug. 27, 2015, 10:54 a.m., Zameer Manji wrote:
> > src/test/java/org/apache/aurora/scheduler/TierManagerTest.java, line 26
> > <https://reviews.apache.org/r/37825/diff/1/?file=1055552#file1055552line26>
> >
> > Please add a test for the config for having keys that are not expected. I would expect the scheduler to reject such files because they are malformed.
>
> Maxim Khutornenko wrote:
> Not sure what you mean by malformed. Are you suggesting adding extra keys or purposefully corrupt json file? The former is irrelevant as unknown keys are ignored on casting to the type and the latter just throws unhandled parsing exception (exactly what we want/expect).
I think if there are extra keys we should reject the file even if the keys are irrelevant. I think operators would want to know if the configuration contains typos or other errors.
- Zameer
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37825/#review96714
-----------------------------------------------------------
On Aug. 27, 2015, 2:17 p.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37825/
> -----------------------------------------------------------
>
> (Updated Aug. 27, 2015, 2:17 p.m.)
>
>
> Review request for Aurora, Bill Farner and Zameer Manji.
>
>
> Bugs: AURORA-1437
> https://issues.apache.org/jira/browse/AURORA-1437
>
>
> Repository: aurora
>
>
> Description
> -------
>
> The external config file is optional for now as tiers are not fully defined yet.
>
>
> Diffs
> -----
>
> examples/vagrant/upstart/aurora-scheduler-kerberos.conf 744b4a35c61e749734e222b3d4cbd296927665aa
> examples/vagrant/upstart/aurora-scheduler.conf 789a3a0315e8530880999432aa9b1e7d0f57d1ff
> src/main/java/org/apache/aurora/scheduler/SchedulerModule.java d4bc6b9bb10c982fb6a2458d7fdd12bddbe98eca
> src/main/java/org/apache/aurora/scheduler/TierManager.java 652afece17a7eb09b0ca68066707b1b8fbf024f0
> src/test/java/org/apache/aurora/scheduler/SchedulerModuleTest.java PRE-CREATION
> src/test/java/org/apache/aurora/scheduler/TierManagerTest.java eb0207114e1b93968cb65832f154a9cd3bc3232e
> src/test/resources/org/apache/aurora/scheduler/tiers-example.json PRE-CREATION
>
> Diff: https://reviews.apache.org/r/37825/diff/
>
>
> Testing
> -------
>
> ./gradlew -Pq build
> ./src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh
>
>
> Thanks,
>
> Maxim Khutornenko
>
>
Re: Review Request 37825: Adding minimal implementation of the
external tier config.
Posted by Maxim Khutornenko <ma...@apache.org>.
> On Aug. 27, 2015, 5:54 p.m., Zameer Manji wrote:
> > src/test/java/org/apache/aurora/scheduler/TierManagerTest.java, line 26
> > <https://reviews.apache.org/r/37825/diff/1/?file=1055552#file1055552line26>
> >
> > Please add a test for the config for having keys that are not expected. I would expect the scheduler to reject such files because they are malformed.
>
> Maxim Khutornenko wrote:
> Not sure what you mean by malformed. Are you suggesting adding extra keys or purposefully corrupt json file? The former is irrelevant as unknown keys are ignored on casting to the type and the latter just throws unhandled parsing exception (exactly what we want/expect).
>
> Zameer Manji wrote:
> I think if there are extra keys we should reject the file even if the keys are irrelevant. I think operators would want to know if the configuration contains typos or other errors.
That's not unreasonable. I'll defer it to AURORA-1443 though as the validation will have to be done on the TierInfo side and would make much more sense when we get to multiple tiers.
- Maxim
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37825/#review96714
-----------------------------------------------------------
On Aug. 27, 2015, 9:17 p.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37825/
> -----------------------------------------------------------
>
> (Updated Aug. 27, 2015, 9:17 p.m.)
>
>
> Review request for Aurora, Bill Farner and Zameer Manji.
>
>
> Bugs: AURORA-1437
> https://issues.apache.org/jira/browse/AURORA-1437
>
>
> Repository: aurora
>
>
> Description
> -------
>
> The external config file is optional for now as tiers are not fully defined yet.
>
>
> Diffs
> -----
>
> examples/vagrant/upstart/aurora-scheduler-kerberos.conf 744b4a35c61e749734e222b3d4cbd296927665aa
> examples/vagrant/upstart/aurora-scheduler.conf 789a3a0315e8530880999432aa9b1e7d0f57d1ff
> src/main/java/org/apache/aurora/scheduler/SchedulerModule.java d4bc6b9bb10c982fb6a2458d7fdd12bddbe98eca
> src/main/java/org/apache/aurora/scheduler/TierManager.java 652afece17a7eb09b0ca68066707b1b8fbf024f0
> src/test/java/org/apache/aurora/scheduler/SchedulerModuleTest.java PRE-CREATION
> src/test/java/org/apache/aurora/scheduler/TierManagerTest.java eb0207114e1b93968cb65832f154a9cd3bc3232e
> src/test/resources/org/apache/aurora/scheduler/tiers-example.json PRE-CREATION
>
> Diff: https://reviews.apache.org/r/37825/diff/
>
>
> Testing
> -------
>
> ./gradlew -Pq build
> ./src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh
>
>
> Thanks,
>
> Maxim Khutornenko
>
>
Re: Review Request 37825: Adding minimal implementation of the
external tier config.
Posted by Maxim Khutornenko <ma...@apache.org>.
> On Aug. 27, 2015, 5:54 p.m., Zameer Manji wrote:
> > src/main/java/org/apache/aurora/scheduler/SchedulerModule.java, line 117
> > <https://reviews.apache.org/r/37825/diff/1/?file=1055550#file1055550line117>
> >
> > I think this function should also parse the data into Map<String, TierInfo>. The earlier we parse data (and error out if data is invalid) the better IMHO.
Yeah, addressed above.
> On Aug. 27, 2015, 5:54 p.m., Zameer Manji wrote:
> > src/main/java/org/apache/aurora/scheduler/TierManager.java, line 69
> > <https://reviews.apache.org/r/37825/diff/1/?file=1055551#file1055551line69>
> >
> > If applying a default value, I think we should log it.
The default tier does not deviate from the current behavior, so logging would not add any additional info here. Besides, this will generate quite a bit of extra log noise as it's used in the scheduling loop.
> On Aug. 27, 2015, 5:54 p.m., Zameer Manji wrote:
> > src/test/java/org/apache/aurora/scheduler/TierManagerTest.java, line 26
> > <https://reviews.apache.org/r/37825/diff/1/?file=1055552#file1055552line26>
> >
> > Please add a test for the config for having keys that are not expected. I would expect the scheduler to reject such files because they are malformed.
Not sure what you mean by malformed. Are you suggesting adding extra keys or purposefully corrupt json file? The former is irrelevant as unknown keys are ignored on casting to the type and the latter just throws unhandled parsing exception (exactly what we want/expect).
- Maxim
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37825/#review96714
-----------------------------------------------------------
On Aug. 27, 2015, 1:07 a.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37825/
> -----------------------------------------------------------
>
> (Updated Aug. 27, 2015, 1:07 a.m.)
>
>
> Review request for Aurora, Bill Farner and Zameer Manji.
>
>
> Bugs: AURORA-1437
> https://issues.apache.org/jira/browse/AURORA-1437
>
>
> Repository: aurora
>
>
> Description
> -------
>
> The external config file is optional for now as tiers are not fully defined yet.
>
>
> Diffs
> -----
>
> examples/vagrant/upstart/aurora-scheduler-kerberos.conf 744b4a35c61e749734e222b3d4cbd296927665aa
> examples/vagrant/upstart/aurora-scheduler.conf 789a3a0315e8530880999432aa9b1e7d0f57d1ff
> src/main/java/org/apache/aurora/scheduler/SchedulerModule.java d4bc6b9bb10c982fb6a2458d7fdd12bddbe98eca
> src/main/java/org/apache/aurora/scheduler/TierManager.java 652afece17a7eb09b0ca68066707b1b8fbf024f0
> src/test/java/org/apache/aurora/scheduler/TierManagerTest.java eb0207114e1b93968cb65832f154a9cd3bc3232e
> src/test/resources/org/apache/aurora/scheduler/tiers-example.json PRE-CREATION
>
> Diff: https://reviews.apache.org/r/37825/diff/
>
>
> Testing
> -------
>
> ./gradlew -Pq build
> ./src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh
>
>
> Thanks,
>
> Maxim Khutornenko
>
>
Re: Review Request 37825: Adding minimal implementation of the
external tier config.
Posted by Zameer Manji <zm...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37825/#review96714
-----------------------------------------------------------
src/main/java/org/apache/aurora/scheduler/SchedulerModule.java (line 117)
<https://reviews.apache.org/r/37825/#comment152355>
I think this function should also parse the data into Map<String, TierInfo>. The earlier we parse data (and error out if data is invalid) the better IMHO.
src/main/java/org/apache/aurora/scheduler/TierManager.java (line 68)
<https://reviews.apache.org/r/37825/#comment152357>
If applying a default value, I think we should log it.
src/test/java/org/apache/aurora/scheduler/TierManagerTest.java (line 26)
<https://reviews.apache.org/r/37825/#comment152356>
Please add a test for the config for having keys that are not expected. I would expect the scheduler to reject such files because they are malformed.
- Zameer Manji
On Aug. 26, 2015, 6:07 p.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37825/
> -----------------------------------------------------------
>
> (Updated Aug. 26, 2015, 6:07 p.m.)
>
>
> Review request for Aurora, Bill Farner and Zameer Manji.
>
>
> Bugs: AURORA-1437
> https://issues.apache.org/jira/browse/AURORA-1437
>
>
> Repository: aurora
>
>
> Description
> -------
>
> The external config file is optional for now as tiers are not fully defined yet.
>
>
> Diffs
> -----
>
> examples/vagrant/upstart/aurora-scheduler-kerberos.conf 744b4a35c61e749734e222b3d4cbd296927665aa
> examples/vagrant/upstart/aurora-scheduler.conf 789a3a0315e8530880999432aa9b1e7d0f57d1ff
> src/main/java/org/apache/aurora/scheduler/SchedulerModule.java d4bc6b9bb10c982fb6a2458d7fdd12bddbe98eca
> src/main/java/org/apache/aurora/scheduler/TierManager.java 652afece17a7eb09b0ca68066707b1b8fbf024f0
> src/test/java/org/apache/aurora/scheduler/TierManagerTest.java eb0207114e1b93968cb65832f154a9cd3bc3232e
> src/test/resources/org/apache/aurora/scheduler/tiers-example.json PRE-CREATION
>
> Diff: https://reviews.apache.org/r/37825/diff/
>
>
> Testing
> -------
>
> ./gradlew -Pq build
> ./src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh
>
>
> Thanks,
>
> Maxim Khutornenko
>
>