Mailing-List: contact reviews-help@mesos.apache.org; run by ezmlm
Precedence: bulk
Reply-To: reviews@mesos.apache.org
Content-Type: multipart/alternative;
 boundary="===============2840243031008443343=="
MIME-Version: 1.0
Subject: Re: Review Request 42289: Calcuated 'remainingClusterResources' by
 all
 activated slaves.
From: "Klaus Ma" <klaus1982.cn@gmail.com>
To: "Joris Van Remoortere" <joris.van.remoortere@gmail.com>,
 "Alexander Rukletsov" <rukletsov@gmail.com>,
 "Neil Conway" <neil.conway@gmail.com>,
 "Ben Mahler" <benjamin.mahler@gmail.com>
Cc: "Klaus Ma" <klaus1982.cn@gmail.com>, "mesos" <reviews@mesos.apache.org>
Date: Tue, 19 Jan 2016 10:57:59 -0000
Message-ID: <20160119105759.32038.65936@reviews.apache.org>
Auto-Submitted: auto-generated
Sender: "Klaus Ma" <noreply@reviews.apache.org>
References: <20160119092349.7986.47135@reviews.apache.org>
In-Reply-To: <20160119092349.7986.47135@reviews.apache.org>
Reply-To: "Klaus Ma" <klaus1982.cn@gmail.com>

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


> On Jan. 19, 2016, 5:23 p.m., Alexander Rukletsov wrote:
> > Let's tweak some wording and testing and we are good to go!
> > 
> > I liked the initial summary more. IMO a patch should describe the solution, and not the problem. It's quite opposite for JIRA tickets, hence I'm convinced patches and tickets should not share the same title. How about "Calcuated 'remainingClusterResources' using all available agents."
> > 
> > I also think it won't hurt to extend the description and explain why we need this change. How about
> > "Event-triggered allocations do not include all available agents. If we
> > calculate remaining resources in the cluster using the partial view,
> > we may overlook already laid away resources for quota'ed roles and lay
> > away more. Hence we may unnecessarily deprive non-quota'ed frameworks
> > of resources."
> > 
> > Changes touching allocator are vulnerable to races, especially in tests. Please extend the testing (and mention this in the "Testing Done" section) with goodies like `GTEST_FILTER="*HierarchicalAllocatorTest*" ./bin/mesos-tests.sh --gtest_shuffle --gtest-repeat`.
> > 
> > Let's add a test for this change. Ideally the test should fail without the change and pass it with the change. I think Neil has already provided the test in the ticket, could you please include it?
> 
> Alexander Rukletsov wrote:
>     Maybe even better, you can modify existing `QuotaAbsentFramework` test.

Yes, that's what I'm thinking :). Current patch passed Neil's test in Mac OS, I'll re-check it in Ubuntu for `./bin/mesos-tests.sh`; it seems perf did not support no-Linux system.


- Klaus


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


On Jan. 19, 2016, 6:55 p.m., Klaus Ma wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42289/
> -----------------------------------------------------------
> 
> (Updated Jan. 19, 2016, 6:55 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Ben Mahler, Joris Van Remoortere, and Neil Conway.
> 
> 
> Bugs: MESOS-4102
>     https://issues.apache.org/jira/browse/MESOS-4102
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> __Phenomenon__:
> Quota doesn't allocate resources on slave joining.
> 
> __Root Cause__:
> Event-triggered allocations do not include all available agents. If we
> calculate remaining resources in the cluster using the partial view,
> we may overlook already laid away resources for quota'ed roles and lay
> away more. Hence we may unnecessarily deprive non-quota'ed frameworks
> of resources.
> 
> Refer to AlexR's comments for more detail:
> https://issues.apache.org/jira/browse/MESOS-4102?focusedCommentId=15048495&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-15048495
> 
> __Solution/Fix__:
> Calcuated 'remainingClusterResources' by all activated slaves.
> 
> 
> Diffs
> -----
> 
>   src/master/allocator/mesos/hierarchical.cpp 48acde69b1a2f305b568a7e322a58708063dd30a 
>   src/tests/hierarchical_allocator_tests.cpp 9362dd306497ba01e0f387c3862456cdcac6f863 
> 
> Diff: https://reviews.apache.org/r/42289/diff/
> 
> 
> Testing
> -------
> 
> make
> make check
> 
> 
> Thanks,
> 
> Klaus Ma
> 
>


--===============2840243031008443343==--