Re: Review Request 42289: Quota doesn't allocate resources on slave joining.

2016-01-19 Thread Alexander Rukletsov 

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


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?


src/master/allocator/mesos/hierarchical.cpp (lines 1227 - 1228)


Let's add a note here that we iterate over all slaves and why. How about

// NOTE: We use all active agents and not just those visible in the current 
`allocate()` call so that frameworks in roles without quota are not 
unnecessarily deprived of resources.


- Alexander Rukletsov


On Jan. 19, 2016, 8:50 a.m., Klaus Ma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42289/
> ---
> 
> (Updated Jan. 19, 2016, 8:50 a.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
> ---
> 
> Calcuated 'remainingClusterResources' by all activated slaves.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.cpp 
> 48acde69b1a2f305b568a7e322a58708063dd30a 
> 
> Diff: https://reviews.apache.org/r/42289/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> 
> 
> Thanks,
> 
> Klaus Ma
> 
>

Re: Review Request 42289: Quota doesn't allocate resources on slave joining.

2016-01-19 Thread Alexander Rukletsov 


> On Jan. 19, 2016, 9:23 a.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?

Maybe even better, you can modify existing `QuotaAbsentFramework` test.


- Alexander


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


On Jan. 19, 2016, 8:50 a.m., Klaus Ma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42289/
> ---
> 
> (Updated Jan. 19, 2016, 8:50 a.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
> ---
> 
> Calcuated 'remainingClusterResources' by all activated slaves.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.cpp 
> 48acde69b1a2f305b568a7e322a58708063dd30a 
> 
> Diff: https://reviews.apache.org/r/42289/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> 
> 
> Thanks,
> 
> Klaus Ma
> 
>