Mailing-List: contact reviews-help@aurora.apache.org; run by ezmlm
Precedence: bulk
Reply-To: reviews@aurora.apache.org
Content-Type: multipart/alternative;
 boundary="===============4576891088024828380=="
MIME-Version: 1.0
Subject: Re: Review Request 42126: New class to allocate resources of multiple
 roles from offer.
From: "Maxim Khutornenko" <maxim@apache.org>
To: "Bill Farner" <wfarner@apache.org>,
 "Maxim Khutornenko" <maxim@apache.org>,
 "Dmitriy Shirchenko" <caldima@gmail.com>
Cc: "Zhitao Li" <zhitaoli.cs@gmail.com>, "Aurora" <reviews@aurora.apache.org>
Date: Wed, 13 Jan 2016 23:01:46 -0000
Message-ID: <20160113230146.26792.24591@reviews.apache.org>
Auto-Submitted: auto-generated
Sender: "Maxim Khutornenko" <noreply@reviews.apache.org>
References: <20160113223236.26792.64718@reviews.apache.org>
In-Reply-To: <20160113223236.26792.64718@reviews.apache.org>
Reply-To: "Maxim Khutornenko" <maxim@apache.org>

--===============4576891088024828380==
MIME-Version: 1.0
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 7bit


-----------------------------------------------------------
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)
<https://reviews.apache.org/r/42126/#comment175108>

    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)
<https://reviews.apache.org/r/42126/#comment175104>

    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)
<https://reviews.apache.org/r/42126/#comment175103>

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


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

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


src/main/java/org/apache/aurora/scheduler/AcceptedOffer.java (lines 182 - 184)
<https://reviews.apache.org/r/42126/#comment175107>

    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)
<https://reviews.apache.org/r/42126/#comment175130>

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


--===============4576891088024828380==--