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

2016-01-14 Thread Bill Farner


> On Jan. 13, 2016, 5:06 p.m., Dmitriy Shirchenko wrote:
> > src/test/java/org/apache/aurora/scheduler/mesos/MesosTaskFactoryImplTest.java,
> >  lines 216-217
> > 
> >
> > Is this intentionally commented out?
> 
> Zhitao Li wrote:
> Yes this is intentially removed. Old behavior of changing this `config` 
> here resulted in `resources` in static final object `OFFER` not sufficient to 
> be allocated, and I don't see any point to use a different executor for this 
> test case, so I've deleted this code.
> 
> If any reviewer thinks there is a reason to test this with a different 
> executor, I can reconstruct an `Offer` object with correct resource and add 
> this back.
> 
> Bill Farner wrote:
> This is slightly suspicious...are you sure resource accounting wasn't 
> impacted in some way?  I would expect old test cases to logically work with 
> this feature.
> 
> Zhitao Li wrote:
> I recalled why I commented this out in the first pass now (this was a bit 
> tricky and I mistakenly thought it's logical bug while it's due to floating 
> point precision).
> 
> In TaskExecutors.java, `SOME_OVERHEAD_EXECUTOR` uses `0.01` cpus:
> ```
>   public static final ExecutorSettings SOME_OVERHEAD_EXECUTOR =
>   TestExecutorSettings.thermosOnlyWithOverhead(
>   new ResourceSlot(0.01, Amount.of(256L, Data.MB), Amount.of(0L, 
> Data.MB), 0));
> ```
> Unfortunately, `5.01 - 5 - 0.01` **is not zero** in floating point math 
> of Java, so the test keeps throwing `InsufficientResourcesException` even if 
> I constructed an `Offer` whose resource is correctly equivalent to 
> `ResourceSlot.from(TASK_CONFIG).add(TaskExecutors.SOME_OVERHEAD_EXECUTOR.getExecutorOverhead())`.
> 
> **My proposal is to change all comparison with zero in `AcceptedOffer` 
> class (3 places in current patch) to compare with an `EPSILON` whose value is 
> `1e-6` or something even smaller.** I don't think there is any reasonable 
> usage of resource unit smaller than this granularity, and the floating point 
> math in the class would be more robust. We can document this behavior 
> somewhere if it sounds interesting.
> 
> The default resource usage for `THERMOS_EXECUTOR` is 0.25 so I don't 
> encounter this elsewhere.
> 
> Some alternatives which I liked less:
> 1. Change `TaskExecutors.SOME_OVERHEAD_EXECUTOR`'s cpus to a value which 
> could be precisely represented by floating point (0.125 or something similar);
> 2. Manually add a bit more cpu resource to the `Offer` object to trick 
> the test to pass.
> 
> If the above proposed solution sounds fine, I can send an update to the 
> patch.

I'm in favor of using an EPSILON value.  Please be sure to extract a function 
to handle the equivalence check.

It's too bad mesos doesn't use fixed point for scalar resources to avoid these 
issues.  Looks like there's been recent momentum to make that change: 
https://issues.apache.org/jira/browse/MESOS-3997

They recently addressed an instance of this issue with this change:
https://github.com/apache/mesos/commit/5b5dd566aea71f654c51b2706a958ca1b5cb07a3

```
CHECK_NEAR(result.cpus().get(), cpus().get(), MIN_CPUS);
```

For reference, `MIN_CPUS` is 0.01.


- Bill


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


On Jan. 13, 2016, 5:39 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42126/
> ---
> 
> (Updated Jan. 13, 2016, 5:39 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 class OfferAllocation, 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
> 
> Several caveats:
> 1. This performs the allocate after scheduling decision in 
> TaskAssigner.maybeAssign is done, which leaves possibility of inconsistency 
> and late failure.
> 
> 
> Diffs
> -
> 
>   NEWS acaff9eb2ab184b0ef750f8b8a00c20131997f6b 
>   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 
> 

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

2016-01-14 Thread Bill Farner

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



src/test/java/org/apache/aurora/scheduler/mesos/MesosTaskFactoryImplTest.java 


I assume this line will be restored once `AcceptedOffer` accommodates for 
floating point errors?


- Bill Farner


On Jan. 13, 2016, 5:39 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42126/
> ---
> 
> (Updated Jan. 13, 2016, 5:39 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 class OfferAllocation, 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
> 
> Several caveats:
> 1. This performs the allocate after scheduling decision in 
> TaskAssigner.maybeAssign is done, which leaves possibility of inconsistency 
> and late failure.
> 
> 
> Diffs
> -
> 
>   NEWS acaff9eb2ab184b0ef750f8b8a00c20131997f6b 
>   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/AcceptedOfferTest.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 class to allocate resources of multiple roles from offer.

2016-01-13 Thread Aurora ReviewBot

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

Ship it!


Master (952ef6d) 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. 13, 2016, 5:50 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42126/
> ---
> 
> (Updated Jan. 13, 2016, 5:50 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 class OfferAllocation, 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
> 
> Several caveats:
> 1. This performs the allocate after scheduling decision in 
> TaskAssigner.maybeAssign is done, which leaves possibility of inconsistency 
> and late failure.
> 
> 
> 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/AcceptedOfferTest.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 class to allocate resources of multiple roles from offer.

2016-01-13 Thread Bill Farner

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

Ship it!


Please also add a NEWS entry so we are sure to highlight this feature in the 
next release!

- Bill Farner


On Jan. 13, 2016, 9:50 a.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42126/
> ---
> 
> (Updated Jan. 13, 2016, 9:50 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 class OfferAllocation, 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
> 
> Several caveats:
> 1. This performs the allocate after scheduling decision in 
> TaskAssigner.maybeAssign is done, which leaves possibility of inconsistency 
> and late failure.
> 
> 
> 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/AcceptedOfferTest.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 class to allocate resources of multiple roles from offer.

2016-01-13 Thread Bill Farner


> On Jan. 13, 2016, 3:01 p.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/scheduler/AcceptedOffer.java, line 79
> > 
> >
> > I'd expect this value populated from the command line arg supplied to 
> > driver setting module. Mind explaining the logic here? Why do we default to 
> > "*"?
> 
> Bill Farner wrote:
> Is this not necessary to align with the special handling of `*` on the 
> mesos side?
> 
> Maxim Khutornenko wrote:
> I guess my question was about what this default even means. I'd expect 
> this feature to be OFF by default by not registering with any role at all. It 
> should be up to cluster operator to decide what role to register with (if at 
> all).

In this context, i believe the logic is appropriate.  Role=`*` means any 
framework may use the resource.  I don't think this is something you would want 
to configure or disable.
https://mesos.apache.org/documentation/latest/roles/

However i agree with you for the role the scheduler registers as (different 
part of this patch).


- Bill


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


On Jan. 13, 2016, 2:32 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42126/
> ---
> 
> (Updated Jan. 13, 2016, 2:32 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 class OfferAllocation, 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
> 
> Several caveats:
> 1. This performs the allocate after scheduling decision in 
> TaskAssigner.maybeAssign is done, which leaves possibility of inconsistency 
> and late failure.
> 
> 
> Diffs
> -
> 
>   NEWS acaff9eb2ab184b0ef750f8b8a00c20131997f6b 
>   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/AcceptedOfferTest.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 class to allocate resources of multiple roles from offer.

2016-01-13 Thread Zhitao Li


> On Jan. 13, 2016, 11:01 p.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/scheduler/AcceptedOffer.java, line 79
> > 
> >
> > I'd expect this value populated from the command line arg supplied to 
> > driver setting module. Mind explaining the logic here? Why do we default to 
> > "*"?
> 
> Bill Farner wrote:
> Is this not necessary to align with the special handling of `*` on the 
> mesos side?
> 
> Maxim Khutornenko wrote:
> I guess my question was about what this default even means. I'd expect 
> this feature to be OFF by default by not registering with any role at all. It 
> should be up to cluster operator to decide what role to register with (if at 
> all).
> 
> Bill Farner wrote:
> In this context, i believe the logic is appropriate.  Role=`*` means any 
> framework may use the resource.  I don't think this is something you would 
> want to configure or disable.
> https://mesos.apache.org/documentation/latest/roles/
> 
> However i agree with you for the role the scheduler registers as 
> (different part of this patch).
> 
> Maxim Khutornenko wrote:
> What is the behavior if someone decides to register with a named role, 
> say "aurora"? Since we are sorting by RESERVED (which is DEFAULT_ROLE_NAME) 
> first, aren't we getting the reverse behavior here? E.g. we'd get anything 
> which is NOT "aurora" first (mostly likely only *) and then "aurora" 
> resources?
> 
> Bill Farner wrote:
> Just to clarify nomenclature, as Zhitao mentioned the naming is off in 
> the current draft.  Unreserved is `*`.
> 
> So the current behavior is to use the unreserved first.

Yes that's the predicate bug I was referring to in my previous reply (still 
getting familiar with the collaboration and replying on review board).

This should be fixed in my last update, and a change in test actually picked 
the behavior up.


- Zhitao


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


On Jan. 13, 2016, 11:50 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42126/
> ---
> 
> (Updated Jan. 13, 2016, 11:50 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 class OfferAllocation, 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
> 
> Several caveats:
> 1. This performs the allocate after scheduling decision in 
> TaskAssigner.maybeAssign is done, which leaves possibility of inconsistency 
> and late failure.
> 
> 
> Diffs
> -
> 
>   NEWS acaff9eb2ab184b0ef750f8b8a00c20131997f6b 
>   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/AcceptedOfferTest.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,
> 
> 

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

2016-01-13 Thread Zhitao Li


> On Jan. 14, 2016, 1:06 a.m., Dmitriy Shirchenko wrote:
> > src/test/java/org/apache/aurora/scheduler/mesos/MesosTaskFactoryImplTest.java,
> >  lines 216-217
> > 
> >
> > Is this intentionally commented out?
> 
> Zhitao Li wrote:
> Yes this is intentially removed. Old behavior of changing this `config` 
> here resulted in `resources` in static final object `OFFER` not sufficient to 
> be allocated, and I don't see any point to use a different executor for this 
> test case, so I've deleted this code.
> 
> If any reviewer thinks there is a reason to test this with a different 
> executor, I can reconstruct an `Offer` object with correct resource and add 
> this back.
> 
> Bill Farner wrote:
> This is slightly suspicious...are you sure resource accounting wasn't 
> impacted in some way?  I would expect old test cases to logically work with 
> this feature.

I recalled why I commented this out in the first pass now (this was a bit 
tricky and I mistakenly thought it's logical bug while it's due to floating 
point precision).

In TaskExecutors.java, `SOME_OVERHEAD_EXECUTOR` uses `0.01` cpus:
```
  public static final ExecutorSettings SOME_OVERHEAD_EXECUTOR =
  TestExecutorSettings.thermosOnlyWithOverhead(
  new ResourceSlot(0.01, Amount.of(256L, Data.MB), Amount.of(0L, 
Data.MB), 0));
```
Unfortunately, `5.01 - 5 - 0.01` **is not zero** in floating point math of 
Java, so the test keeps throwing `InsufficientResourcesException` even if I 
constructed an `Offer` whose resource is correctly equivalent to 
`ResourceSlot.from(TASK_CONFIG).add(TaskExecutors.SOME_OVERHEAD_EXECUTOR.getExecutorOverhead())`.

**My proposal is to change all comparison with zero in `AcceptedOffer` class (3 
places in current patch) to compare with an `EPSILON` whose value is `1e-6` or 
something even smaller.** I don't think there is any reasonable usage of 
resource unit smaller than this granularity, and the floating point math in the 
class would be more robust. We can document this behavior somewhere if it 
sounds interesting.

The default resource usage for `THERMOS_EXECUTOR` is 0.25 so I don't encounter 
this elsewhere.

Some alternatives which I liked less:
1. Change `TaskExecutors.SOME_OVERHEAD_EXECUTOR`'s cpus to a value which could 
be precisely represented by floating point (0.125 or something similar);
2. Manually add a bit more cpu resource to the `Offer` object to trick the test 
to pass.

If the above proposed solution sounds fine, I can send an update to the patch.


- Zhitao


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


On Jan. 14, 2016, 1:39 a.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42126/
> ---
> 
> (Updated Jan. 14, 2016, 1:39 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 class OfferAllocation, 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
> 
> Several caveats:
> 1. This performs the allocate after scheduling decision in 
> TaskAssigner.maybeAssign is done, which leaves possibility of inconsistency 
> and late failure.
> 
> 
> Diffs
> -
> 
>   NEWS acaff9eb2ab184b0ef750f8b8a00c20131997f6b 
>   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/AcceptedOfferTest.java 
> PRE-CREATION 
>   src/test/java/org/apache/aurora/scheduler/ResourceSlotTest.java 
> e4ae943303823ac4bfbe999ed22f5999484462d8 
>   
> src/test/java/org/apache/aurora/scheduler/mesos/CommandLineDriverSettingsModuleTest.java
>  33149ab415292eff04f38b61f2b1d1eac79f347a 
>   
> 

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

2016-01-13 Thread Zhitao Li

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

(Updated Jan. 13, 2016, 10:02 p.m.)


Review request for Aurora, Maxim Khutornenko, Dmitriy Shirchenko, and Bill 
Farner.


Changes
---

Add an entry to the NEWS section for next release.


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 class OfferAllocation, 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

Several caveats:
1. This performs the allocate after scheduling decision in 
TaskAssigner.maybeAssign is done, which leaves possibility of inconsistency and 
late failure.


Diffs (updated)
-

  NEWS e2b26d9513c15451afb0766fa0aa6f534f6afe49 
  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/AcceptedOfferTest.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 class to allocate resources of multiple roles from offer.

2016-01-13 Thread Zhitao Li

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

(Updated Jan. 13, 2016, 10:32 p.m.)


Review request for Aurora, Maxim Khutornenko, Dmitriy Shirchenko, and Bill 
Farner.


Changes
---

Rebase and merge changes in NEWS.


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 class OfferAllocation, 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

Several caveats:
1. This performs the allocate after scheduling decision in 
TaskAssigner.maybeAssign is done, which leaves possibility of inconsistency and 
late failure.


Diffs (updated)
-

  NEWS acaff9eb2ab184b0ef750f8b8a00c20131997f6b 
  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/AcceptedOfferTest.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 class to allocate resources of multiple roles from offer.

2016-01-13 Thread Aurora ReviewBot

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

Ship it!


Master (952ef6d) 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. 13, 2016, 10:32 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42126/
> ---
> 
> (Updated Jan. 13, 2016, 10:32 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 class OfferAllocation, 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
> 
> Several caveats:
> 1. This performs the allocate after scheduling decision in 
> TaskAssigner.maybeAssign is done, which leaves possibility of inconsistency 
> and late failure.
> 
> 
> Diffs
> -
> 
>   NEWS acaff9eb2ab184b0ef750f8b8a00c20131997f6b 
>   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/AcceptedOfferTest.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 class to allocate resources of multiple roles from offer.

2016-01-13 Thread Bill Farner


> On Jan. 13, 2016, 3:01 p.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/scheduler/AcceptedOffer.java, line 79
> > 
> >
> > I'd expect this value populated from the command line arg supplied to 
> > driver setting module. Mind explaining the logic here? Why do we default to 
> > "*"?

Is this not necessary to align with the special handling of `*` on the mesos 
side?


> On Jan. 13, 2016, 3:01 p.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/scheduler/AcceptedOffer.java, lines 182-184
> > 
> >
> > Does not Mesos expect resource role set when accepting an offer? 
> > Otherwise, how does it know what resource (from what role) is actually 
> > accepted?

`r.build()` accomplishes this by carrying the role value forward, right?


- Bill


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


On Jan. 13, 2016, 2:32 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42126/
> ---
> 
> (Updated Jan. 13, 2016, 2:32 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 class OfferAllocation, 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
> 
> Several caveats:
> 1. This performs the allocate after scheduling decision in 
> TaskAssigner.maybeAssign is done, which leaves possibility of inconsistency 
> and late failure.
> 
> 
> Diffs
> -
> 
>   NEWS acaff9eb2ab184b0ef750f8b8a00c20131997f6b 
>   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/AcceptedOfferTest.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 class to allocate resources of multiple roles from offer.

2016-01-13 Thread Maxim Khutornenko


> On Jan. 13, 2016, 11:01 p.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/scheduler/AcceptedOffer.java, lines 182-184
> > 
> >
> > Does not Mesos expect resource role set when accepting an offer? 
> > Otherwise, how does it know what resource (from what role) is actually 
> > accepted?
> 
> Bill Farner wrote:
> `r.build()` accomplishes this by carrying the role value forward, right?

You're right. Completely missed 'r' there.


- Maxim


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


On Jan. 13, 2016, 10:32 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42126/
> ---
> 
> (Updated Jan. 13, 2016, 10:32 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 class OfferAllocation, 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
> 
> Several caveats:
> 1. This performs the allocate after scheduling decision in 
> TaskAssigner.maybeAssign is done, which leaves possibility of inconsistency 
> and late failure.
> 
> 
> Diffs
> -
> 
>   NEWS acaff9eb2ab184b0ef750f8b8a00c20131997f6b 
>   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/AcceptedOfferTest.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 class to allocate resources of multiple roles from offer.

2016-01-13 Thread Zhitao Li

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



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


I noticed that this predicate is actually reversed (sorry for the bug).

Fixing in next update.


- Zhitao Li


On Jan. 13, 2016, 10:32 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42126/
> ---
> 
> (Updated Jan. 13, 2016, 10:32 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 class OfferAllocation, 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
> 
> Several caveats:
> 1. This performs the allocate after scheduling decision in 
> TaskAssigner.maybeAssign is done, which leaves possibility of inconsistency 
> and late failure.
> 
> 
> Diffs
> -
> 
>   NEWS acaff9eb2ab184b0ef750f8b8a00c20131997f6b 
>   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/AcceptedOfferTest.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 class to allocate resources of multiple roles from offer.

2016-01-13 Thread Bill Farner


> On Jan. 13, 2016, 3:01 p.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/scheduler/AcceptedOffer.java, line 79
> > 
> >
> > I'd expect this value populated from the command line arg supplied to 
> > driver setting module. Mind explaining the logic here? Why do we default to 
> > "*"?
> 
> Bill Farner wrote:
> Is this not necessary to align with the special handling of `*` on the 
> mesos side?
> 
> Maxim Khutornenko wrote:
> I guess my question was about what this default even means. I'd expect 
> this feature to be OFF by default by not registering with any role at all. It 
> should be up to cluster operator to decide what role to register with (if at 
> all).
> 
> Bill Farner wrote:
> In this context, i believe the logic is appropriate.  Role=`*` means any 
> framework may use the resource.  I don't think this is something you would 
> want to configure or disable.
> https://mesos.apache.org/documentation/latest/roles/
> 
> However i agree with you for the role the scheduler registers as 
> (different part of this patch).
> 
> Maxim Khutornenko wrote:
> What is the behavior if someone decides to register with a named role, 
> say "aurora"? Since we are sorting by RESERVED (which is DEFAULT_ROLE_NAME) 
> first, aren't we getting the reverse behavior here? E.g. we'd get anything 
> which is NOT "aurora" first (mostly likely only *) and then "aurora" 
> resources?

Just to clarify nomenclature, as Zhitao mentioned the naming is off in the 
current draft.  Unreserved is `*`.

So the current behavior is to use the unreserved first.


- Bill


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


On Jan. 13, 2016, 3:50 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42126/
> ---
> 
> (Updated Jan. 13, 2016, 3:50 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 class OfferAllocation, 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
> 
> Several caveats:
> 1. This performs the allocate after scheduling decision in 
> TaskAssigner.maybeAssign is done, which leaves possibility of inconsistency 
> and late failure.
> 
> 
> Diffs
> -
> 
>   NEWS acaff9eb2ab184b0ef750f8b8a00c20131997f6b 
>   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/AcceptedOfferTest.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 class to allocate resources of multiple roles from offer.

2016-01-13 Thread Maxim Khutornenko

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



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


I'd expect this value populated from the command line arg supplied to 
driver setting module. Mind explaining the logic here? Why do we default to "*"?



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


I'd put private and final fields to the top as this better follows our 
usual arrangement.



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


+newline after last arg here and in other places where args spill over.



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


Pull 'throws' to the previous line and insert newline after it



src/main/java/org/apache/aurora/scheduler/AcceptedOffer.java (lines 182 - 184)


Does not Mesos expect resource role set when accepting an offer? Otherwise, 
how does it know what resource (from what role) is actually accepted?



src/main/java/org/apache/aurora/scheduler/mesos/CommandLineDriverSettingsModule.java
 (line 94)


Does it have to be a default here? IMO, the less intrusive approach would 
be allow it to be empty and not populate framework role at all. This could be 
similar to FRAMEWORK_AUTHENTICATION_FILE approach and would result in a zero 
risk upgrade cycle.


- Maxim Khutornenko


On Jan. 13, 2016, 10:32 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42126/
> ---
> 
> (Updated Jan. 13, 2016, 10:32 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 class OfferAllocation, 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
> 
> Several caveats:
> 1. This performs the allocate after scheduling decision in 
> TaskAssigner.maybeAssign is done, which leaves possibility of inconsistency 
> and late failure.
> 
> 
> Diffs
> -
> 
>   NEWS acaff9eb2ab184b0ef750f8b8a00c20131997f6b 
>   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/AcceptedOfferTest.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 class to allocate resources of multiple roles from offer.

2016-01-13 Thread Maxim Khutornenko


> On Jan. 13, 2016, 11:01 p.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/scheduler/AcceptedOffer.java, line 79
> > 
> >
> > I'd expect this value populated from the command line arg supplied to 
> > driver setting module. Mind explaining the logic here? Why do we default to 
> > "*"?
> 
> Bill Farner wrote:
> Is this not necessary to align with the special handling of `*` on the 
> mesos side?
> 
> Maxim Khutornenko wrote:
> I guess my question was about what this default even means. I'd expect 
> this feature to be OFF by default by not registering with any role at all. It 
> should be up to cluster operator to decide what role to register with (if at 
> all).
> 
> Bill Farner wrote:
> In this context, i believe the logic is appropriate.  Role=`*` means any 
> framework may use the resource.  I don't think this is something you would 
> want to configure or disable.
> https://mesos.apache.org/documentation/latest/roles/
> 
> However i agree with you for the role the scheduler registers as 
> (different part of this patch).

What is the behavior if someone decides to register with a named role, say 
"aurora"? Since we are sorting by RESERVED (which is DEFAULT_ROLE_NAME) first, 
aren't we getting the reverse behavior here? E.g. we'd get anything which is 
NOT "aurora" first (mostly likely only *) and then "aurora" resources?


- Maxim


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


On Jan. 13, 2016, 11:50 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42126/
> ---
> 
> (Updated Jan. 13, 2016, 11:50 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 class OfferAllocation, 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
> 
> Several caveats:
> 1. This performs the allocate after scheduling decision in 
> TaskAssigner.maybeAssign is done, which leaves possibility of inconsistency 
> and late failure.
> 
> 
> Diffs
> -
> 
>   NEWS acaff9eb2ab184b0ef750f8b8a00c20131997f6b 
>   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/AcceptedOfferTest.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 class to allocate resources of multiple roles from offer.

2016-01-13 Thread Aurora ReviewBot

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


This patch does not apply cleanly against master (952ef6d), do you need to 
rebase?

I will refresh this build result if you post a review containing "@ReviewBot 
retry"

- Aurora ReviewBot


On Jan. 13, 2016, 10:02 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42126/
> ---
> 
> (Updated Jan. 13, 2016, 10:02 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 class OfferAllocation, 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
> 
> Several caveats:
> 1. This performs the allocate after scheduling decision in 
> TaskAssigner.maybeAssign is done, which leaves possibility of inconsistency 
> and late failure.
> 
> 
> Diffs
> -
> 
>   NEWS e2b26d9513c15451afb0766fa0aa6f534f6afe49 
>   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/AcceptedOfferTest.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 class to allocate resources of multiple roles from offer.

2016-01-13 Thread Maxim Khutornenko


> On Jan. 13, 2016, 11:01 p.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/scheduler/AcceptedOffer.java, line 79
> > 
> >
> > I'd expect this value populated from the command line arg supplied to 
> > driver setting module. Mind explaining the logic here? Why do we default to 
> > "*"?
> 
> Bill Farner wrote:
> Is this not necessary to align with the special handling of `*` on the 
> mesos side?

I guess my question was about what this default even means. I'd expect this 
feature to be OFF by default by not registering with any role at all. It should 
be up to cluster operator to decide what role to register with (if at all).


- Maxim


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


On Jan. 13, 2016, 10:32 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42126/
> ---
> 
> (Updated Jan. 13, 2016, 10:32 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 class OfferAllocation, 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
> 
> Several caveats:
> 1. This performs the allocate after scheduling decision in 
> TaskAssigner.maybeAssign is done, which leaves possibility of inconsistency 
> and late failure.
> 
> 
> Diffs
> -
> 
>   NEWS acaff9eb2ab184b0ef750f8b8a00c20131997f6b 
>   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/AcceptedOfferTest.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 class to allocate resources of multiple roles from offer.

2016-01-13 Thread Bill Farner


> On Jan. 13, 2016, 3:48 p.m., Zhitao Li wrote:
> > src/main/java/org/apache/aurora/scheduler/AcceptedOffer.java, line 85
> > 
> >
> > I noticed that this predicate is actually reversed (sorry for the bug).
> > 
> > Fixing in next update.

Thanks, I was trying to comment on that but RB on a mobile web browser is 
clumsy at best :-)


- Bill


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


On Jan. 13, 2016, 3:50 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42126/
> ---
> 
> (Updated Jan. 13, 2016, 3:50 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 class OfferAllocation, 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
> 
> Several caveats:
> 1. This performs the allocate after scheduling decision in 
> TaskAssigner.maybeAssign is done, which leaves possibility of inconsistency 
> and late failure.
> 
> 
> Diffs
> -
> 
>   NEWS acaff9eb2ab184b0ef750f8b8a00c20131997f6b 
>   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/AcceptedOfferTest.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 class to allocate resources of multiple roles from offer.

2016-01-13 Thread Zhitao Li

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

(Updated Jan. 13, 2016, 11:50 p.m.)


Review request for Aurora, Maxim Khutornenko, Dmitriy Shirchenko, and Bill 
Farner.


Changes
---

Fix incorrect `RESERVED` predicate, and all comments from Maxim.


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 class OfferAllocation, 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

Several caveats:
1. This performs the allocate after scheduling decision in 
TaskAssigner.maybeAssign is done, which leaves possibility of inconsistency and 
late failure.


Diffs (updated)
-

  NEWS acaff9eb2ab184b0ef750f8b8a00c20131997f6b 
  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/AcceptedOfferTest.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 class to allocate resources of multiple roles from offer.

2016-01-13 Thread Aurora ReviewBot

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

Ship it!


Master (952ef6d) 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. 13, 2016, 11:50 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42126/
> ---
> 
> (Updated Jan. 13, 2016, 11:50 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 class OfferAllocation, 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
> 
> Several caveats:
> 1. This performs the allocate after scheduling decision in 
> TaskAssigner.maybeAssign is done, which leaves possibility of inconsistency 
> and late failure.
> 
> 
> Diffs
> -
> 
>   NEWS acaff9eb2ab184b0ef750f8b8a00c20131997f6b 
>   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/AcceptedOfferTest.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 class to allocate resources of multiple roles from offer.

2016-01-13 Thread Bill Farner


> On Jan. 13, 2016, 4:57 p.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/scheduler/mesos/CommandLineDriverSettingsModule.java,
> >  lines 93-94
> > 
> >
> > This does not read right to me. If it's empty, a framework will be 
> > registered without any role at all.

+1


- Bill


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


On Jan. 13, 2016, 3:50 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42126/
> ---
> 
> (Updated Jan. 13, 2016, 3:50 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 class OfferAllocation, 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
> 
> Several caveats:
> 1. This performs the allocate after scheduling decision in 
> TaskAssigner.maybeAssign is done, which leaves possibility of inconsistency 
> and late failure.
> 
> 
> Diffs
> -
> 
>   NEWS acaff9eb2ab184b0ef750f8b8a00c20131997f6b 
>   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/AcceptedOfferTest.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 class to allocate resources of multiple roles from offer.

2016-01-13 Thread Aurora ReviewBot

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

Ship it!


Master (952ef6d) 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. 14, 2016, 1:39 a.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42126/
> ---
> 
> (Updated Jan. 14, 2016, 1:39 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 class OfferAllocation, 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
> 
> Several caveats:
> 1. This performs the allocate after scheduling decision in 
> TaskAssigner.maybeAssign is done, which leaves possibility of inconsistency 
> and late failure.
> 
> 
> Diffs
> -
> 
>   NEWS acaff9eb2ab184b0ef750f8b8a00c20131997f6b 
>   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/AcceptedOfferTest.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 class to allocate resources of multiple roles from offer.

2016-01-13 Thread Dmitriy Shirchenko

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



src/test/java/org/apache/aurora/scheduler/mesos/MesosTaskFactoryImplTest.java 
(lines 216 - 217)


Is this intentionally commented out?


- Dmitriy Shirchenko


On Jan. 13, 2016, 11:50 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42126/
> ---
> 
> (Updated Jan. 13, 2016, 11:50 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 class OfferAllocation, 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
> 
> Several caveats:
> 1. This performs the allocate after scheduling decision in 
> TaskAssigner.maybeAssign is done, which leaves possibility of inconsistency 
> and late failure.
> 
> 
> Diffs
> -
> 
>   NEWS acaff9eb2ab184b0ef750f8b8a00c20131997f6b 
>   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/AcceptedOfferTest.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 class to allocate resources of multiple roles from offer.

2016-01-13 Thread Maxim Khutornenko

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


Only minor comments left.


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


Suggest moving all static final fields to the top of this class.



src/main/java/org/apache/aurora/scheduler/mesos/CommandLineDriverSettingsModule.java
 (lines 93 - 94)


This does not read right to me. If it's empty, a framework will be 
registered without any role at all.


- Maxim Khutornenko


On Jan. 13, 2016, 11:50 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42126/
> ---
> 
> (Updated Jan. 13, 2016, 11:50 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 class OfferAllocation, 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
> 
> Several caveats:
> 1. This performs the allocate after scheduling decision in 
> TaskAssigner.maybeAssign is done, which leaves possibility of inconsistency 
> and late failure.
> 
> 
> Diffs
> -
> 
>   NEWS acaff9eb2ab184b0ef750f8b8a00c20131997f6b 
>   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/AcceptedOfferTest.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 class to allocate resources of multiple roles from offer.

2016-01-13 Thread Zhitao Li


> On Jan. 14, 2016, 12:57 a.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/scheduler/mesos/CommandLineDriverSettingsModule.java,
> >  lines 93-94
> > 
> >
> > This does not read right to me. If it's empty, a framework will be 
> > registered without any role at all.
> 
> Bill Farner wrote:
> +1

Will fix to "`The Mesos role this framework will register as. If left empty, 
this framework will register without any role and only receive unreserved 
resources in offers.`"


> On Jan. 14, 2016, 12:57 a.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/scheduler/AcceptedOffer.java, line 82
> > 
> >
> > Suggest moving all static final fields to the top of this class.

Will do.


- Zhitao


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


On Jan. 13, 2016, 11:50 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42126/
> ---
> 
> (Updated Jan. 13, 2016, 11:50 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 class OfferAllocation, 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
> 
> Several caveats:
> 1. This performs the allocate after scheduling decision in 
> TaskAssigner.maybeAssign is done, which leaves possibility of inconsistency 
> and late failure.
> 
> 
> Diffs
> -
> 
>   NEWS acaff9eb2ab184b0ef750f8b8a00c20131997f6b 
>   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/AcceptedOfferTest.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 class to allocate resources of multiple roles from offer.

2016-01-13 Thread Zhitao Li


> On Jan. 14, 2016, 1:06 a.m., Dmitriy Shirchenko wrote:
> > src/test/java/org/apache/aurora/scheduler/mesos/MesosTaskFactoryImplTest.java,
> >  lines 216-217
> > 
> >
> > Is this intentionally commented out?

Yes this is intentially removed. Old behavior of changing this `config` here 
resulted in `resources` in static final object `OFFER` not sufficient to be 
allocated, and I don't see any point to use a different executor for this test 
case, so I've deleted this code.

If any reviewer thinks there is a reason to test this with a different 
executor, I can reconstruct an `Offer` object with correct resource and add 
this back.


- Zhitao


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


On Jan. 14, 2016, 1:39 a.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42126/
> ---
> 
> (Updated Jan. 14, 2016, 1:39 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 class OfferAllocation, 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
> 
> Several caveats:
> 1. This performs the allocate after scheduling decision in 
> TaskAssigner.maybeAssign is done, which leaves possibility of inconsistency 
> and late failure.
> 
> 
> Diffs
> -
> 
>   NEWS acaff9eb2ab184b0ef750f8b8a00c20131997f6b 
>   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/AcceptedOfferTest.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 class to allocate resources of multiple roles from offer.

2016-01-13 Thread Zhitao Li


> On Jan. 14, 2016, 12:57 a.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/scheduler/mesos/CommandLineDriverSettingsModule.java,
> >  lines 93-94
> > 
> >
> > This does not read right to me. If it's empty, a framework will be 
> > registered without any role at all.
> 
> Bill Farner wrote:
> +1
> 
> Zhitao Li wrote:
> Will fix to "`The Mesos role this framework will register as. If left 
> empty, this framework will register without any role and only receive 
> unreserved resources in offers.`"
> 
> Bill Farner wrote:
> To be clear, I believe Maxim and I are advocating a behavior change - the 
> default should be to _not_ set a role (as opposed to role `*`).

I think this is more clear: "`The Mesos role this framework will register as. 
The default is to left this empty, and the framework will register without any 
role and only receive unreserved resources (role = "*") in offer.`"


- Zhitao


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


On Jan. 13, 2016, 11:50 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42126/
> ---
> 
> (Updated Jan. 13, 2016, 11:50 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 class OfferAllocation, 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
> 
> Several caveats:
> 1. This performs the allocate after scheduling decision in 
> TaskAssigner.maybeAssign is done, which leaves possibility of inconsistency 
> and late failure.
> 
> 
> Diffs
> -
> 
>   NEWS acaff9eb2ab184b0ef750f8b8a00c20131997f6b 
>   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/AcceptedOfferTest.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 class to allocate resources of multiple roles from offer.

2016-01-13 Thread Bill Farner


> On Jan. 13, 2016, 5:06 p.m., Dmitriy Shirchenko wrote:
> > src/test/java/org/apache/aurora/scheduler/mesos/MesosTaskFactoryImplTest.java,
> >  lines 216-217
> > 
> >
> > Is this intentionally commented out?
> 
> Zhitao Li wrote:
> Yes this is intentially removed. Old behavior of changing this `config` 
> here resulted in `resources` in static final object `OFFER` not sufficient to 
> be allocated, and I don't see any point to use a different executor for this 
> test case, so I've deleted this code.
> 
> If any reviewer thinks there is a reason to test this with a different 
> executor, I can reconstruct an `Offer` object with correct resource and add 
> this back.

This is slightly suspicious...are you sure resource accounting wasn't impacted 
in some way?  I would expect old test cases to logically work with this feature.


- Bill


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


On Jan. 13, 2016, 5:39 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42126/
> ---
> 
> (Updated Jan. 13, 2016, 5:39 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 class OfferAllocation, 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
> 
> Several caveats:
> 1. This performs the allocate after scheduling decision in 
> TaskAssigner.maybeAssign is done, which leaves possibility of inconsistency 
> and late failure.
> 
> 
> Diffs
> -
> 
>   NEWS acaff9eb2ab184b0ef750f8b8a00c20131997f6b 
>   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/AcceptedOfferTest.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 class to allocate resources of multiple roles from offer.

2016-01-13 Thread Zhitao Li


> On Jan. 13, 2016, 2:57 a.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/AcceptedOffer.java, line 129
> > 
> >
> > This is a good general-purpose implementation of this behavior, but in 
> > practice it seems like overkill.  Can you instead match the behavior 
> > currently on master and synthesize the `Resource` with 
> > `makeMesosRangeResource(PORTS, selectedPorts)`?

I think we still need this function, because ports can be reserved to a 
particular role, similar to cpus/mem/disk.

In practice, it seems possible to me that cluster operator decided to partition 
available port range among differnet frameworks. Certain Mesos framework 
(Cassandra) even requires fixed ports in resource offers to launch their 
workload, because their workload only works well with these fixed ports.

Given such possibility, it seems we still need to have correct role for the 
ports used when sending them back to Mesos, mostly for handling all possible 
configuration purpose.

Please let me know if you think otherwise.


- Zhitao


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


On Jan. 13, 2016, 2:08 a.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42126/
> ---
> 
> (Updated Jan. 13, 2016, 2:08 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 class OfferAllocation, 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
> 
> Several caveats:
> 1. This performs the allocate after scheduling decision in 
> TaskAssigner.maybeAssign is done, which leaves possibility of inconsistency 
> and late failure.
> 
> 
> 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/AcceptedOfferTest.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 class to allocate resources of multiple roles from offer.

2016-01-13 Thread Zhitao Li

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

(Updated Jan. 13, 2016, 5:50 p.m.)


Review request for Aurora, Maxim Khutornenko, Dmitriy Shirchenko, and Bill 
Farner.


Changes
---

All review comments from this round, except the one about 
`makeMesosRangeResource`, which I replied separately.


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 class OfferAllocation, 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

Several caveats:
1. This performs the allocate after scheduling decision in 
TaskAssigner.maybeAssign is done, which leaves possibility of inconsistency and 
late failure.


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/AcceptedOfferTest.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 class to allocate resources of multiple roles from offer.

2016-01-12 Thread Aurora ReviewBot

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

Ship it!


Master (952ef6d) 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. 13, 2016, 2:08 a.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42126/
> ---
> 
> (Updated Jan. 13, 2016, 2:08 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 class OfferAllocation, 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
> 
> Several caveats:
> 1. This performs the allocate after scheduling decision in 
> TaskAssigner.maybeAssign is done, which leaves possibility of inconsistency 
> and late failure.
> 
> 
> 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/AcceptedOfferTest.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 class to allocate resources of multiple roles from offer.

2016-01-12 Thread Bill Farner

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


Thanks for the changes so far, nearly there!


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


I did a quick rearrangement of this method (after applying the comment 
below), i think the result is a bit easier to read, and uses fewer temp 
variables.

I also cheated and omitted the `requireNonNull` calls as nulls will 
immediately trigger NPE for all parameters.

```
  public static AcceptedOffer create(
  Offer offer,
  ResourceSlot taskSlot,
  ResourceSlot executorSlot,
  Set selectedPorts,
  TierInfo tierInfo) throws Resources.InsufficientResourcesException {

List reservedFirst = ImmutableList.builder()
.addAll(Iterables.filter(offer.getResourcesList(), RESERVED))
.addAll(Iterables.filter(offer.getResourcesList(), NOT_RESERVED))
.build();

boolean revocable = tierInfo.isRevocable();
List cpuResources = filterToBuilders(
reservedFirst,
ResourceType.CPUS.getName(),
revocable ? Resources.REVOCABLE : Resources.NON_REVOCABLE);
List memResources = filterToBuilderNonRevocable(
reservedFirst, ResourceType.RAM_MB.getName());
List diskResources = filterToBuilderNonRevocable(
reservedFirst, ResourceType.DISK_MB.getName());
List portsResources = filterToBuilderNonRevocable(
reservedFirst, ResourceType.PORTS.getName());

List taskResources = ImmutableList.builder()
.addAll(allocateScalarType(cpuResources, taskSlot.getNumCpus(), 
revocable))
.addAll(allocateScalarType(memResources, 
taskSlot.getRam().as(Data.MB), false))
.addAll(allocateScalarType(diskResources, 
taskSlot.getDisk().as(Data.MB), false))
.addAll(allocateRangeType(portsResources, selectedPorts))
.build();

List executorResources = ImmutableList.builder()
.addAll(allocateScalarType(cpuResources, executorSlot.getNumCpus(), 
revocable))
.addAll(allocateScalarType(memResources, 
executorSlot.getRam().as(Data.MB), false))
.addAll(allocateScalarType(diskResources, 
executorSlot.getDisk().as(Data.MB), false))
.build();

return new AcceptedOffer(taskResources, executorResources);
  }
```



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


feel free to declare as `List` instead of `ImmutableList`, ditto in the 
constructor.



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


This is a good general-purpose implementation of this behavior, but in 
practice it seems like overkill.  Can you instead match the behavior currently 
on master and synthesize the `Resource` with `makeMesosRangeResource(PORTS, 
selectedPorts)`?



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


Remove `final` here and throughout the method.  We only use `final` when 
necessary, or to ensure immutability of class field references.



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


This parameter is now effectively a return value, so instead make it a 
formal return type.



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


`remaining` instead of `outstanding`



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


Move this up to the line after the declaration of `used` to make for easier 
readability of the accounting logic.



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


`r.getScalarBuilder().setValue(available - used);`



src/test/java/org/apache/aurora/scheduler/AcceptedOfferTest.java (line 154)


`runMultipleRolesTest` to make it clear this is not intended to be a test 
case



src/test/java/org/apache/aurora/scheduler/AcceptedOfferTest.java (line 253)


You could simplify this a bit for callers by changing `Set... 
values` to `Integer... values`.  This way callers don't need the 
`ImmutableSet.of()` boilerplate.  It also resolves the unchecked generic vararg 
compiler warning you have now.

On this end, you will remove the `for` loop and do:
 

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

2016-01-12 Thread Zhitao Li

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

(Updated Jan. 13, 2016, 1:34 a.m.)


Review request for Aurora, Maxim Khutornenko, Dmitriy Shirchenko, and Bill 
Farner.


Changes
---

Better code and test organized by Will's comments, making all lists in the 
review immutable.

Also updatd review summyar and description.


Summary (updated)
-

New class to allocate resources of multiple roles from offer.


Bugs: AURORA-1109
https://issues.apache.org/jira/browse/AURORA-1109


Repository: aurora


Description (updated)
---

This review is a prototype for introducing multiple role support in Aurora.
This creates a new class OfferAllocation, 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

Several caveats:
1. This performs the allocate after scheduling decision in 
TaskAssigner.maybeAssign is done, which leaves possibility of inconsistency and 
late failure.


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/AcceptedOfferTest.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 class to allocate resources of multiple roles from offer.

2016-01-12 Thread Bill Farner

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



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


Looks like an overly-aggressive find/replace.  I've been endlessly 
frustrated by intellij doing this when you forget to untick the "search text 
and comments" option (which is always ticked).

Please skim through the patch to find other spots impacted.


- Bill Farner


On Jan. 12, 2016, 5:34 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42126/
> ---
> 
> (Updated Jan. 12, 2016, 5:34 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 class OfferAllocation, 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
> 
> Several caveats:
> 1. This performs the allocate after scheduling decision in 
> TaskAssigner.maybeAssign is done, which leaves possibility of inconsistency 
> and late failure.
> 
> 
> 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/AcceptedOfferTest.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 class to allocate resources of multiple roles from offer.

2016-01-12 Thread John Sirois


> On Jan. 12, 2016, 6:49 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/Resources.java, line 50
> > 
> >
> > Looks like an overly-aggressive find/replace.  I've been endlessly 
> > frustrated by intellij doing this when you forget to untick the "search 
> > text and comments" option (which is always ticked).
> > 
> > Please skim through the patch to find other spots impacted.

In IntelliJ settings, search for "Find Usages Settings" - I map this to 
`ctrl-alt-shift-7` - pops up dialog with scope and seach in text or not and 
other options.


- John


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


On Jan. 12, 2016, 6:34 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42126/
> ---
> 
> (Updated Jan. 12, 2016, 6:34 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 class OfferAllocation, 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
> 
> Several caveats:
> 1. This performs the allocate after scheduling decision in 
> TaskAssigner.maybeAssign is done, which leaves possibility of inconsistency 
> and late failure.
> 
> 
> 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/AcceptedOfferTest.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 class to allocate resources of multiple roles from offer.

2016-01-12 Thread Aurora ReviewBot

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

Ship it!


Master (952ef6d) 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. 13, 2016, 1:34 a.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42126/
> ---
> 
> (Updated Jan. 13, 2016, 1:34 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 class OfferAllocation, 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
> 
> Several caveats:
> 1. This performs the allocate after scheduling decision in 
> TaskAssigner.maybeAssign is done, which leaves possibility of inconsistency 
> and late failure.
> 
> 
> 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/AcceptedOfferTest.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 class to allocate resources of multiple roles from offer.

2016-01-12 Thread John Sirois


> On Jan. 12, 2016, 6:49 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/Resources.java, line 50
> > 
> >
> > Looks like an overly-aggressive find/replace.  I've been endlessly 
> > frustrated by intellij doing this when you forget to untick the "search 
> > text and comments" option (which is always ticked).
> > 
> > Please skim through the patch to find other spots impacted.
> 
> John Sirois wrote:
> In IntelliJ settings, search for "Find Usages Settings" - I map this to 
> `ctrl-alt-shift-7` - pops up dialog with scope and seach in text or not and 
> other options.

Oh - oops, you're issue is having to untick.  I managed to set things up so its 
unticked by default, I'll dig.


- John


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


On Jan. 12, 2016, 6:34 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42126/
> ---
> 
> (Updated Jan. 12, 2016, 6:34 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 class OfferAllocation, 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
> 
> Several caveats:
> 1. This performs the allocate after scheduling decision in 
> TaskAssigner.maybeAssign is done, which leaves possibility of inconsistency 
> and late failure.
> 
> 
> 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/AcceptedOfferTest.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 class to allocate resources of multiple roles from offer.

2016-01-12 Thread Zhitao Li

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

(Updated Jan. 13, 2016, 2:08 a.m.)


Review request for Aurora, Maxim Khutornenko, Dmitriy Shirchenko, and Bill 
Farner.


Changes
---

Undo incorrect comment find/replace by Intellij.


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 class OfferAllocation, 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

Several caveats:
1. This performs the allocate after scheduling decision in 
TaskAssigner.maybeAssign is done, which leaves possibility of inconsistency and 
late failure.


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