----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30010/#review69688 -----------------------------------------------------------
Looks great to me, thanks for retaining the legacy behavior! I'm good to give a ship once these small nits are addressed. src/main/java/org/apache/aurora/scheduler/configuration/ConfigurationManager.java <https://reviews.apache.org/r/30010/#comment114412> Please flip this to act in the affirmative. Arguments that act as double-negative are confusing, especially at 3 AM while a cluster is on fire. src/main/java/org/apache/aurora/scheduler/configuration/ConfigurationManager.java <https://reviews.apache.org/r/30010/#comment114417> s/public // src/main/java/org/apache/aurora/scheduler/configuration/ConfigurationManager.java <https://reviews.apache.org/r/30010/#comment114418> empty line between javadoc body and tags src/main/java/org/apache/aurora/scheduler/configuration/ConfigurationManager.java <https://reviews.apache.org/r/30010/#comment114419> s/public // src/main/java/org/apache/aurora/scheduler/configuration/ConfigurationManager.java <https://reviews.apache.org/r/30010/#comment114420> Comment style should be a space between comment characters and comment text, and always in a complete sentence with punctuation. In this case, the line just needs the space at the front and a period at the end. There are a few lines needing this treatment in the test as well. src/test/java/org/apache/aurora/scheduler/configuration/ConfigurationManagerTest.java <https://reviews.apache.org/r/30010/#comment114421> 'By default, legacy constraints are applied to production services.' src/test/java/org/apache/aurora/scheduler/configuration/ConfigurationManagerTest.java <https://reviews.apache.org/r/30010/#comment114423> We typically don't do this: ``` //isLegacyDisabled=true ``` Ideally we could do this in code with named parameters, but alas the language does not permit it. I propose removing the comment since it is not immune to refactors, and deviates from the rest of the codebase. - Bill Farner On Jan. 25, 2015, 8:10 p.m., Florian Pfeiffer wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/30010/ > ----------------------------------------------------------- > > (Updated Jan. 25, 2015, 8:10 p.m.) > > > Review request for Aurora, Bill Farner and Zameer Manji. > > > Bugs: AURORA-184 > https://issues.apache.org/jira/browse/AURORA-184 > > > Repository: aurora > > > Description > ------- > > [AURORA-184] Remove hardcoded 'host' and 'rack' limit constraints > > This is the first step for AURORA-184, that removes the default host&rack > limit constraints. > The second step that's still missing would be to add s.th. like > "--default-constraints" as start parameter to the scheduler. > > AURORA-174 could probably be closed with this?(since the rack limit > constraint can be configured in the .aurora file) > > I can't really estimate the effect of my changes in > StorageBackfillTest&SchedulerThriftInterfaceTest, please have a closer look > at the changes I did there. > > Since this is also my first code submit, comments about codestyle&other bad > habbits are very appreciated. > > > Diffs > ----- > > > src/main/java/org/apache/aurora/scheduler/configuration/ConfigurationManager.java > 5dfbcf1f6de716502a28f7da33a095968eb8420e > > src/test/java/org/apache/aurora/scheduler/configuration/ConfigurationManagerTest.java > 92ba45033ada8114349c435316c9681395aea706 > > Diff: https://reviews.apache.org/r/30010/diff/ > > > Testing > ------- > > Added test for ConfigurationManager.hasName > Added test testNoHostAndRackConstraintsAdded, that checks if the constraints > are present > Tested on vagrant devcluster to see if constraints are also gone in "real > life" > > > Thanks, > > Florian Pfeiffer > >
