> On Jan. 12, 2016, 6:05 p.m., Bill Farner wrote: > > src/main/java/org/apache/aurora/scheduler/AcceptedOffer.java, line 40 > > <https://reviews.apache.org/r/42126/diff/2/?file=1192873#file1192873line40> > > > > The interface should really be removed. It's not acting as an > > interface hiding an implementation, so its value is not worth its weight. > > Everything else can remain as-is (e.g. static factory function).
Will do. > On Jan. 12, 2016, 6:05 p.m., Bill Farner wrote: > > src/main/java/org/apache/aurora/scheduler/AcceptedOffer.java, line 62 > > <https://reviews.apache.org/r/42126/diff/2/?file=1192873#file1192873line62> > > > > Please move ~all of this method body into `create` to avoid doing > > significant work in the constructor. > > > > Background on this convention can be found here if you're interested: > > http://misko.hevery.com/code-reviewers-guide/flaw-constructor-does-real-work/ > > > > The constructor should whittle down to something as basic as: > > > > ``` > > AcceptedOffer(ImmutableList<Resource> taskResources, > > ImmutableList<Resource> executorResources) { > > this.taskResources = requireNonNull(taskResources); > > this.executorResources = requireNonNull(executorResources); > > } > > ``` Will do. > On Jan. 12, 2016, 6:05 p.m., Bill Farner wrote: > > src/main/java/org/apache/aurora/scheduler/AcceptedOffer.java, line 199 > > <https://reviews.apache.org/r/42126/diff/2/?file=1192873#file1192873line199> > > > > I now see why you do indeed need _some_ flag for `revocable` because > > both `REVOCABLE` and `NON_REVOCABLE` filters are active (neither are a > > no-op). I suggest that you accept `Predicate<Resource>` and introduce an > > overload to make the call sites more readable (`false` does not indicate > > much) > > > > ``` > > static List<Resource> filterNonRevocableAndSort(List<Resource> > > resources, String name) { > > return filterAndSort( > > resources, > > name, > > revocable ? Resources.REVOCABLE : Resources.NON_REVOCABLE); > > } > > > > static List<Resource> filterAndSort( > > List<Resource> resources, > > String name, > > Predicate<Resource> additionalFilter) { > > > > return FluentIterable.from(resources) > > .filter(e -> e.getName().equals(name)) > > .filter(additionalFilter) > > .toSortedList(PREFER_RESERVED); > > } > > ``` > > > > Then most call sites become `filterNonRevocableAndSort`, and the call > > for CPU passes `Resources.REVOCABLE`. Does that make sense? Yes this makes sense. The only difference I'd like is to make this return an `ArrayList` instead of `ImmutableList` from `toSortedList` because this list needs to be mutable so we can allocate resources from it. > On Jan. 12, 2016, 6:05 p.m., Bill Farner wrote: > > src/main/java/org/apache/aurora/scheduler/AcceptedOffer.java, lines 76-97 > > <https://reviews.apache.org/r/42126/diff/2/?file=1192873#file1192873line76> > > > > A few thoughts for this code block: > > 1. rather than sort, filter reserved resources and arrange them first. > > the sort is extra work, and currently defines an arbitrary sub-sort on role > > name which is misleading at best. > > 2. convert to List<Resource.Builder> upfront and use these to track the > > remaining capacity of the resource by mutating them > > > > If you agree, you would start this block with something like > > ``` > > List<Resource> reservedFirst = ImmutableList.<Resource>builder() > > .addAll(Iterables.filter(offer.getResourcesList(), RESERVED) > > .addAll(Iterables.filter(offer.getResourcesList(), NOT_RESERVED) > > .build() > > > > List<Resource.Builder> availableResources = > > FluentIterable.from(reservedFirst) > > .transform(Resource::toBuilder) > > .toList(); > > ``` > > > > This will also allow you to avoid modifying the `List` itself (as is > > currently done below with `ListIterator.remove/add`) since you can adjust > > the remaining capacity in `Resource.Builder` instances rather than > > constructing new ones. Will do. > On Jan. 12, 2016, 6:05 p.m., Bill Farner wrote: > > src/main/java/org/apache/aurora/scheduler/AcceptedOffer.java, line 110 > > <https://reviews.apache.org/r/42126/diff/2/?file=1192873#file1192873line110> > > > > I'd like to strive to remove `@VisibleForTesting` from all methods in > > this class and make them `private`. It will likely require more > > fixtures/helpers in the unit test, but the result will be a unit test that > > is not so highly coupled to the implementation. Do you see anything > > preventing this approach? Will do. I think I just need one helper function to be package private so I don't repeat it in test. - Zhitao ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/42126/#review114000 ----------------------------------------------------------- On Jan. 12, 2016, 2:29 a.m., Zhitao Li wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/42126/ > ----------------------------------------------------------- > > (Updated Jan. 12, 2016, 2:29 a.m.) > > > Review request for Aurora, Maxim Khutornenko, Dmitriy Shirchenko, and Bill > Farner. > > > Bugs: AURORA-1109 > https://issues.apache.org/jira/browse/AURORA-1109 > > > Repository: aurora > > > Description > ------- > > This review is a prototype for introducing multiple role support in Aurora. > This creates a new interface OfferAllocation (with implementation), which > allcoates resources to resources field in TaskInfo and ExecutorInfo from an > offer. > > Current implementation prefers reserved resources over shared resources ('*' > role) if both are present. This can be customized with a differnet > comparator from PREFER_RESERVED in the future when needed. > > Several caveats: > 1. This performs the allocate after scheduling decision in > TaskAssigner.maybeAssign is done, which leaves possibility of inconsistency > and late failure; > 2. Old code used to construct resources has not been cleaned up yet; > 3. OfferAllocationImpl's constructor allows to throw, which is a bit awkward. > > > Diffs > ----- > > src/main/java/org/apache/aurora/scheduler/AcceptedOffer.java PRE-CREATION > src/main/java/org/apache/aurora/scheduler/ResourceSlot.java > 7c3d681c216b78eeecebbe950186e5a79c6fe982 > src/main/java/org/apache/aurora/scheduler/Resources.java > db422a959ee7b982c2a44323de41ad75d1a40754 > > src/main/java/org/apache/aurora/scheduler/mesos/CommandLineDriverSettingsModule.java > 2255dd407cd1810c7df5baf17cfa85f79bfffeb8 > src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java > 8fdadda67478bb3110aa442b7d78493cf9c3edb4 > src/main/java/org/apache/aurora/scheduler/state/TaskAssigner.java > 7e8e456e288986eb0ce92a123b294e1e25d8ed18 > src/test/java/org/apache/aurora/scheduler/AcceptedOfferImplTest.java > PRE-CREATION > src/test/java/org/apache/aurora/scheduler/ResourceSlotTest.java > e4ae943303823ac4bfbe999ed22f5999484462d8 > > src/test/java/org/apache/aurora/scheduler/mesos/CommandLineDriverSettingsModuleTest.java > 33149ab415292eff04f38b61f2b1d1eac79f347a > > src/test/java/org/apache/aurora/scheduler/mesos/MesosTaskFactoryImplTest.java > a5793bffabf4e5d6195b1b99f2363d241c0cecf9 > src/test/java/org/apache/aurora/scheduler/state/TaskAssignerImplTest.java > 3cbe9acd75def14ae2e0986914ba621fb164b3e4 > > Diff: https://reviews.apache.org/r/42126/diff/ > > > Testing > ------- > > 1. Unit tested with old and new tests; > 2. vagrant integration tests: I manually separate out the vagrant box's cpu > and memory between 'aurora-test' role and '*' and verified that jobs can > still be launched (I can post the vagrant change in another follow upon > request). > > > Thanks, > > Zhitao Li > >
