Re: Review Request 42126: New interface to allocate resources of multiple roles from offer.

[Bill Farner]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42126/#review114000
---



src/main/java/org/apache/aurora/scheduler/AcceptedOffer.java (line 40)


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



src/main/java/org/apache/aurora/scheduler/AcceptedOffer.java (line 62)


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 taskResources, 
ImmutableList executorResources) {
  this.taskResources = requireNonNull(taskResources);
  this.executorResources = requireNonNull(executorResources);
}
```



src/main/java/org/apache/aurora/scheduler/AcceptedOffer.java (lines 76 - 97)


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 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 reservedFirst = ImmutableList.builder()
  .addAll(Iterables.filter(offer.getResourcesList(), RESERVED)
  .addAll(Iterables.filter(offer.getResourcesList(), NOT_RESERVED)
  .build()

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



src/main/java/org/apache/aurora/scheduler/AcceptedOffer.java (line 110)


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?



src/main/java/org/apache/aurora/scheduler/AcceptedOffer.java (line 199)


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` and introduce an overload to make 
the call sites more readable (`false` does not indicate much)

```
static List filterNonRevocableAndSort(List resources, 
String name) {
  return filterAndSort(
  resources,
  name,
  revocable ? Resources.REVOCABLE : Resources.NON_REVOCABLE);
}

static List filterAndSort(
List resources,
String name,
Predicate 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?


- Bill Farner


On Jan. 11, 2016, 6:29 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42126/
> ---
> 
> (Updated Jan. 11, 2016, 6:29 p.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 

Re: Review Request 42126: New interface to allocate resources of multiple roles from offer.

[Bill Farner]

> On Jan. 11, 2016, 3:42 p.m., Zhitao Li wrote:
> > src/main/java/org/apache/aurora/scheduler/OfferAllocation.java, line 188
> > 
> >
> > Hmm, I think I'd like to keep this for two reasons:
> > 
> > 1. Even though only one callsite uses value other than `false`, the 
> > `false` value still dictates a `Predicate` to use, so we'll have more code 
> > duplicate if we move this out;
> > 2. Right now aurora only supports `cpus` as revocable resource. I 
> > actually wanted to discuss this separately as current prediction in our 
> > company indicates that we might be more memory bounded. In such a case, we 
> > could be using memory as revocable resources, too.

> Even though only one callsite uses value other than false, the false value 
> still dictates a Predicate to use, so we'll have more code duplicate if we 
> move this out

Not sure i agree with the code duplication, but i will follow up on the next 
patch draft.

> ...we might be more memory bounded. In such a case, we could be using memory 
> as revocable resources, too.

I'm not sure if that is possible, but it definitely is not recommended.

http://mesos.apache.org/documentation/latest/oversubscription/
> It is recommended only to oversubscribe compressible resources such as cpu 
> shares, bandwidth, etc.


> On Jan. 11, 2016, 3:42 p.m., Zhitao Li wrote:
> > src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java, line 
> > 120
> > 
> >
> > I don't plan on any further usage of the interface other than here in 
> > this series of patches.
> > 
> > The only possible customization that could happen is the reverse 
> > ordering of resource preference (`'*'` vs `reserved role`) but I'd like to 
> > withhold it until someone request such feature.
> > 
> > I think I'll keep the interface definition, change the 
> > `AcceptedOfferImpl` to package private, and use a factory method anyway. We 
> > can revisit this later. How does this sound?

I will follow up on the latest draft.


- Bill


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42126/#review113862
---


On Jan. 11, 2016, 6:29 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42126/
> ---
> 
> (Updated Jan. 11, 2016, 6:29 p.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 
> 

Re: Review Request 42126: New interface to allocate resources of multiple roles from offer.

[Zhitao Li]

> On Jan. 12, 2016, 6:05 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/AcceptedOffer.java, line 40
> > 
> >
> > 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
> > 
> >
> > 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 taskResources, 
> > ImmutableList 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
> > 
> >
> > 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` and introduce an 
> > overload to make the call sites more readable (`false` does not indicate 
> > much)
> > 
> > ```
> > static List filterNonRevocableAndSort(List 
> > resources, String name) {
> >   return filterAndSort(
> >   resources,
> >   name,
> >   revocable ? Resources.REVOCABLE : Resources.NON_REVOCABLE);
> > }
> > 
> > static List filterAndSort(
> > List resources,
> > String name,
> > Predicate 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
> > 
> >
> > 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 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 reservedFirst = ImmutableList.builder()
> >   .addAll(Iterables.filter(offer.getResourcesList(), RESERVED)
> >   .addAll(Iterables.filter(offer.getResourcesList(), NOT_RESERVED)
> >   .build()
> > 
> > List 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
> > 
> >
> > 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

Re: Review Request 42126: New interface to allocate resources of multiple roles from offer.

[Bill Farner]

> On Jan. 11, 2016, 8:25 a.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java, line 
> > 201
> > 
> >
> > Is `clearResources()` necessary here?  I think it makes sense to not 
> > intervene here if an operator with a custom config wants to specify 
> > resources, even if it's at their peril.
> 
> Zhitao Li wrote:
> This is at least necessary to get current tests pass. IIRC this is due to 
> how Aurora handles resource limit of executor right now:
> 1. A prototype of `ExecutorInfo` is created in `ExecutorSettings`;
> 2. A `ResourceSlot` representing the usage is computed right before when 
> we need to allocate resources from an offer;
> 3. The `ResourceSlot` is then passed to my new implementation to allocate 
> resources properly next to `TaskInfo`'s.
> 
> We would need to change this workflow to make sure there cannot be 
> previously set resources if we want to avoid the `clearResources()` call.
> 
> Let me know if you think tackling this workflow change should be part of 
> this change.

Sounds reasonable.  I suspect we will need to revisit this in the future, but 
probably not worth dwelling on now.


- Bill


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42126/#review113768
---


On Jan. 11, 2016, 6:29 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42126/
> ---
> 
> (Updated Jan. 11, 2016, 6:29 p.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
> 
>

Re: Review Request 42126: New interface to allocate resources of multiple roles from offer.

[Zhitao Li]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42126/#review113862
---



src/main/java/org/apache/aurora/scheduler/OfferAllocation.java (line 188)


Hmm, I think I'd like to keep this for two reasons:

1. Even though only one callsite uses value other than `false`, the `false` 
value still dictates a `Predicate` to use, so we'll have more code duplicate if 
we move this out;
2. Right now aurora only supports `cpus` as revocable resource. I actually 
wanted to discuss this separately as current prediction in our company 
indicates that we might be more memory bounded. In such a case, we could be 
using memory as revocable resources, too.



src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java (line 120)


I don't plan on any further usage of the interface other than here in this 
series of patches.

The only possible customization that could happen is the reverse ordering 
of resource preference (`'*'` vs `reserved role`) but I'd like to withhold it 
until someone request such feature.

I think I'll keep the interface definition, change the `AcceptedOfferImpl` 
to package private, and use a factory method anyway. We can revisit this later. 
How does this sound?


- Zhitao Li


On Jan. 11, 2016, 8:22 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42126/
> ---
> 
> (Updated Jan. 11, 2016, 8:22 p.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/OfferAllocation.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/MesosTaskFactory.java 
> 8fdadda67478bb3110aa442b7d78493cf9c3edb4 
>   src/main/java/org/apache/aurora/scheduler/state/TaskAssigner.java 
> 7e8e456e288986eb0ce92a123b294e1e25d8ed18 
>   src/test/java/org/apache/aurora/scheduler/OfferAllocationImplTest.java 
> PRE-CREATION 
>   
> 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
> ---
> 
> Only testing with new and existing unit test. I'll work on more integration 
> test once we decide that this is the proper direction.
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 42126: New interface to allocate resources of multiple roles from offer.

[Zhitao Li]

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


Changes
---

- Review comments
- Adding 'mesos_role' to command line argument.


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 (updated)
-

  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 (updated)
---

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

Re: Review Request 42126: New interface to allocate resources of multiple roles from offer.

[Aurora ReviewBot]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42126/#review113917
---

Ship it!


Master (f064dc1) 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 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
> 
>

Re: Review Request 42126: New interface to allocate resources of multiple roles from offer.

[Bill Farner]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42126/#review113768
---


Nice job!  Most comments here are nits to align code style/conventions.  I've 
mostly left `OfferAllocation.java` alone in this pass, but you should consider 
applying the spirit of these comments there (specifically immutability, testing 
public interface).

Also, the name `OfferAllocation` doesn't sound quite right.  How about 
`AcceptedOffer`?  I don't have a strong opinion here, names are hard and highly 
subjective :-)


src/main/java/org/apache/aurora/scheduler/OfferAllocation.java (line 188)


Consider handling `revocable` with a filter on the result of this function 
rather than an argument since only one caller passes something other than 
`false`.



src/main/java/org/apache/aurora/scheduler/ResourceSlot.java (line 100)


+1 here and below, consider doing that in this patch or an immediate 
follow-up if you'd like to minimize the patch delta.  Your call.



src/main/java/org/apache/aurora/scheduler/Resources.java (line 157)


Reminder to remove this



src/main/java/org/apache/aurora/scheduler/Resources.java (line 169)


Always prefer immutable when it makes sense.  For this function, it's a 
matter of changing to something like:

```
ImmutableList.Builder ranges = ImmutableList.builder();
...

ranges.addAll(Iterables.concat(r.getRanges().getRangeList().iterator()));
...
return ranges.build();
``



src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java (line 105)


Nit: our style is to only use the `final` keyword when necessary, or for 
class fields (for reference immutability).

s/final //



src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java (line 106)


This block of code doesn't do much explicit validation of fields within the 
protobuf, so IMHO you can safely remove this line.



src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java (line 120)


IMHO this line removes the value of the interface indirection.  Are there 
follow-ups to this patch where you intend other bodies of code will only rely 
on the interface?



src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java (line 128)


Rather than having the constructor throw, consider creating a factory 
function that creates an `OfferAllocation`.  Additionally, please declare 
`throws InsufficientResourcesException` on the method signature.



src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java (line 131)


ditto re: final



src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java (line 201)


Is `clearResources()` necessary here?  I think it makes sense to not 
intervene here if an operator with a custom config wants to specify resources, 
even if it's at their peril.



src/test/java/org/apache/aurora/scheduler/OfferAllocationImplTest.java (line 49)


I suggest you use the non-builder types in test cases.  Mutability can make 
these types of tests become brittle over time.  With a helper function, you 
should be able to produce an equally-concise test case that uses all immutable 
`Resource` objects.

```
private static Resource makeResouce(String role) {
  ...
}
```



src/test/java/org/apache/aurora/scheduler/OfferAllocationImplTest.java (line 69)


I generally prefer to avoid accessing private functions for the purposes of 
tests (unless it's prohibitive).  Seems like we can infer correct sort/filter 
behavior from the contents of `OfferAllocation` instances.  Does it make sense 
to only use the public API in that case?



src/test/java/org/apache/aurora/scheduler/OfferAllocationImplTest.java (line 82)


`ImmutableList.of()` can stand in for `Lists.newArrayList()` here and below.



src/test/java/org/apache/aurora/scheduler/OfferAllocationImplTest.java (line 85)


The index-based access is probably not going to age well, especially given 
that the `addAll` calls above can technically add mulitple elements.  While it 
will be more verbose, consider using variables for each of values 

Re: Review Request 42126: New interface to allocate resources of multiple roles from offer.

[Joshua Cohen]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42126/#review113785
---



src/main/java/org/apache/aurora/scheduler/OfferAllocation.java (lines 56 - 60)


One more style nit: our style for contination indents is to indent four 
spaces rather than align method args. Additionally a continuation-indented line 
should be immediately followed by a blank line. Which is to say, this (and 
other instances of aligned method args) should be formatted as:

public OfferAllocationImpl(
Offer offer,
ResourceSlot taskSlot,
...
TierInfo tierInfo) {

  requireNonNull(offer);
  ...


- Joshua Cohen


On Jan. 11, 2016, 5:41 a.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42126/
> ---
> 
> (Updated Jan. 11, 2016, 5:41 a.m.)
> 
> 
> Review request for Aurora, Maxim Khutornenko 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/OfferAllocation.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/MesosTaskFactory.java 
> 8fdadda67478bb3110aa442b7d78493cf9c3edb4 
>   src/main/java/org/apache/aurora/scheduler/state/TaskAssigner.java 
> 7e8e456e288986eb0ce92a123b294e1e25d8ed18 
>   src/test/java/org/apache/aurora/scheduler/OfferAllocationImplTest.java 
> PRE-CREATION 
>   
> 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
> ---
> 
> Only testing with new and existing unit test. I'll work on more integration 
> test once we decide that this is the proper direction.
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 42126: New interface to allocate resources of multiple roles from offer.

[Aurora ReviewBot]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42126/#review113681
---

Ship it!


Master (e4c9c73) 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 Jan. 11, 2016, 5:41 a.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42126/
> ---
> 
> (Updated Jan. 11, 2016, 5:41 a.m.)
> 
> 
> Review request for Aurora, Maxim Khutornenko 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/OfferAllocation.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/MesosTaskFactory.java 
> 8fdadda67478bb3110aa442b7d78493cf9c3edb4 
>   src/main/java/org/apache/aurora/scheduler/state/TaskAssigner.java 
> 7e8e456e288986eb0ce92a123b294e1e25d8ed18 
>   src/test/java/org/apache/aurora/scheduler/OfferAllocationImplTest.java 
> PRE-CREATION 
>   
> 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
> ---
> 
> Only testing with new and existing unit test. I'll work on more integration 
> test once we decide that this is the proper direction.
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>