> 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
> 
>

Reply via email to