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


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

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


src/main/java/org/apache/aurora/scheduler/OfferAllocation.java (line 188)
<https://reviews.apache.org/r/42126/#comment174594>

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



src/main/java/org/apache/aurora/scheduler/ResourceSlot.java (line 100)
<https://reviews.apache.org/r/42126/#comment174578>

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



src/main/java/org/apache/aurora/scheduler/Resources.java (line 157)
<https://reviews.apache.org/r/42126/#comment174579>

    Reminder to remove this



src/main/java/org/apache/aurora/scheduler/Resources.java (line 169)
<https://reviews.apache.org/r/42126/#comment174580>

    Always prefer immutable when it makes sense.  For this function, it's a 
matter of changing to something like:
    
    ```
    ImmutableList.Builder<Range> ranges = ImmutableList.builder();
    ...
    
    ranges.addAll(Iterables.concat(r.getRanges().getRangeList().iterator()));
    ...
    return ranges.build();
    ``



src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java (line 105)
<https://reviews.apache.org/r/42126/#comment174576>

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



src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java (line 106)
<https://reviews.apache.org/r/42126/#comment174577>

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



src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java (line 120)
<https://reviews.apache.org/r/42126/#comment174573>

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



src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java (line 128)
<https://reviews.apache.org/r/42126/#comment174574>

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



src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java (line 131)
<https://reviews.apache.org/r/42126/#comment174575>

    ditto re: final



src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java (line 201)
<https://reviews.apache.org/r/42126/#comment174581>

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



src/test/java/org/apache/aurora/scheduler/OfferAllocationImplTest.java (line 49)
<https://reviews.apache.org/r/42126/#comment174584>

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



src/test/java/org/apache/aurora/scheduler/OfferAllocationImplTest.java (line 69)
<https://reviews.apache.org/r/42126/#comment174589>

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



src/test/java/org/apache/aurora/scheduler/OfferAllocationImplTest.java (line 82)
<https://reviews.apache.org/r/42126/#comment174586>

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



src/test/java/org/apache/aurora/scheduler/OfferAllocationImplTest.java (line 85)
<https://reviews.apache.org/r/42126/#comment174587>

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


- Bill Farner


On Jan. 10, 2016, 9:41 p.m., Zhitao Li wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42126/
> -----------------------------------------------------------
> 
> (Updated Jan. 10, 2016, 9:41 p.m.)
> 
> 
> Review request for Aurora, Maxim Khutornenko and Bill Farner.
> 
> 
> Bugs: AURORA-1109
>     https://issues.apache.org/jira/browse/AURORA-1109
> 
> 
> Repository: aurora
> 
> 
> Description
> -------
> 
> This review is a prototype for introducing multiple role support in Aurora.
> This creates a new interface OfferAllocation (with implementation), which 
> allcoates resources to resources field in TaskInfo and ExecutorInfo from an 
> offer.
> 
> Current implementation prefers reserved resources over shared resources ('*' 
> role) if both are present. This can be  customized with a differnet 
> comparator from PREFER_RESERVED in the future when needed.
> 
> Several caveats:
> 1. This performs the allocate after scheduling decision in 
> TaskAssigner.maybeAssign is done, which leaves possibility of inconsistency 
> and late failure;
> 2. Old code used to construct resources has not been cleaned up yet;
> 3. OfferAllocationImpl's constructor allows to throw, which is a bit awkward.
> 
> 
> Diffs
> -----
> 
>   src/main/java/org/apache/aurora/scheduler/OfferAllocation.java PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/ResourceSlot.java 
> 7c3d681c216b78eeecebbe950186e5a79c6fe982 
>   src/main/java/org/apache/aurora/scheduler/Resources.java 
> db422a959ee7b982c2a44323de41ad75d1a40754 
>   src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java 
> 8fdadda67478bb3110aa442b7d78493cf9c3edb4 
>   src/main/java/org/apache/aurora/scheduler/state/TaskAssigner.java 
> 7e8e456e288986eb0ce92a123b294e1e25d8ed18 
>   src/test/java/org/apache/aurora/scheduler/OfferAllocationImplTest.java 
> PRE-CREATION 
>   
> src/test/java/org/apache/aurora/scheduler/mesos/MesosTaskFactoryImplTest.java 
> a5793bffabf4e5d6195b1b99f2363d241c0cecf9 
>   src/test/java/org/apache/aurora/scheduler/state/TaskAssignerImplTest.java 
> 3cbe9acd75def14ae2e0986914ba621fb164b3e4 
> 
> Diff: https://reviews.apache.org/r/42126/diff/
> 
> 
> Testing
> -------
> 
> Only testing with new and existing unit test. I'll work on more integration 
> test once we decide that this is the proper direction.
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Reply via email to