Re: Review Request 53096: Fix handling in shared count in total resources in the sorter.

2017-02-03 Thread Anindya Sinha 

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

(Updated Feb. 3, 2017, 10:01 p.m.)


Review request for mesos and Jiang Yan Xu.


Changes
---

Refactor.


Bugs: MESOS-6444
https://issues.apache.org/jira/browse/MESOS-6444


Repository: mesos


Description
---

We maintain a single copy of shared resource in the role and quota
sorter's total resources. So, when we update these resources, we
remove the previous resources at this agent and add the new resources
at this agent.


Diffs (updated)
-

  src/master/allocator/mesos/hierarchical.hpp 
a99ed35169c0a4a1db3ac3b9b09f510f8fcddbfb 
  src/master/allocator/mesos/hierarchical.cpp 
5f540569043df9d2bb75416c8c36bb4dd7bd68a1 
  src/tests/persistent_volume_tests.cpp 
468a85b4a6ce09592341afd07ce12a03f5fc4f73 

Diff: https://reviews.apache.org/r/53096/diff/


Testing
---

Tests passed.


Thanks,

Anindya Sinha

Re: Review Request 53096: Fix handling in shared count in total resources in the sorter.

2017-02-01 Thread Anindya Sinha 

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

(Updated Feb. 1, 2017, 10:45 p.m.)


Review request for mesos and Jiang Yan Xu.


Changes
---

Rebased after batch allocation patch was merged.


Bugs: MESOS-6444
https://issues.apache.org/jira/browse/MESOS-6444


Repository: mesos


Description
---

We maintain a single copy of shared resource in the role and quota
sorter's total resources. So, when we update these resources, we
remove the previous resources at this agent and add the new resources
at this agent.


Diffs (updated)
-

  src/master/allocator/mesos/hierarchical.hpp 
339b3d2bada526fd66d812df30e9615a96883769 
  src/master/allocator/mesos/hierarchical.cpp 
ffa07087f9ed8d28b99cc4cde7c739cfd7edb1e1 
  src/tests/persistent_volume_tests.cpp 
468a85b4a6ce09592341afd07ce12a03f5fc4f73 

Diff: https://reviews.apache.org/r/53096/diff/


Testing
---

Tests passed.


Thanks,

Anindya Sinha

Re: Review Request 53096: Fix handling in shared count in total resources in the sorter.

2017-01-30 Thread Anindya Sinha 

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

(Updated Jan. 31, 2017, 5:58 a.m.)


Review request for mesos and Jiang Yan Xu.


Changes
---

Rebased.


Bugs: MESOS-6444
https://issues.apache.org/jira/browse/MESOS-6444


Repository: mesos


Description
---

We maintain a single copy of shared resource in the role and quota
sorter's total resources. So, when we update these resources, we
remove the previous resources at this agent and add the new resources
at this agent.


Diffs (updated)
-

  src/master/allocator/mesos/hierarchical.hpp 
2cda3e311ce339d82065d68de83e86439fa192ff 
  src/master/allocator/mesos/hierarchical.cpp 
f471b6848bebae601a7a0509e9c6ad5eab4fa4a2 
  src/tests/persistent_volume_tests.cpp 
468a85b4a6ce09592341afd07ce12a03f5fc4f73 

Diff: https://reviews.apache.org/r/53096/diff/


Testing
---

Tests passed.


Thanks,

Anindya Sinha

Re: Review Request 53096: Fix handling in shared count in total resources in the sorter.

2017-01-26 Thread Anindya Sinha 

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

(Updated Jan. 26, 2017, 10:06 p.m.)


Review request for mesos and Jiang Yan Xu.


Changes
---

Addressed review comments.


Bugs: MESOS-6444
https://issues.apache.org/jira/browse/MESOS-6444


Repository: mesos


Description
---

We maintain a single copy of shared resource in the role and quota
sorter's total resources. So, when we update these resources, we
remove the previous resources at this agent and add the new resources
at this agent.


Diffs (updated)
-

  src/master/allocator/mesos/hierarchical.hpp 
9b66c23f26b37c02ed850c06c4518ea99078b02d 
  src/master/allocator/mesos/hierarchical.cpp 
c2211be7458755aeb91ef078e4bfe92ac474044a 
  src/tests/persistent_volume_tests.cpp 
468a85b4a6ce09592341afd07ce12a03f5fc4f73 

Diff: https://reviews.apache.org/r/53096/diff/


Testing
---

Tests passed.


Thanks,

Anindya Sinha

Re: Review Request 53096: Fix handling in shared count in total resources in the sorter.

2017-01-25 Thread Jiang Yan Xu 

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


Fix it, then Ship it!




This review is now closely related to the followup review /r/55359/ which 
almost makes sense to combine them. But because this patch fixes a bug (with 
tests to prove that) I guess we'll still keep them separate. Just thought I'd 
mention it for posterity.


src/master/allocator/mesos/hierarchical.hpp (line 479)


As mentioned in the reply from the previous conversation:

s/updateTotal/updateSlaveTotal/

And the comments:

```
// Helper to update the agent's total resources maintained in the allocator 
and the role
// and quota sorters (whose total resources match the agent's total 
resources).
```



src/master/allocator/mesos/hierarchical.cpp (line 720)


s/total/agent's total/



src/master/allocator/mesos/hierarchical.cpp (line 754)


s/allocated resources/allocation/ 

It's synonymous but `allocation` is more consistent with the comments above 
(now that we don't need to say "total and allocated resources").

Same applies to another occurrence below.



src/master/allocator/mesos/hierarchical.cpp (line 1977)


Can we also add the following sentense before "So, we update them using the 
resources in `slaves[slaveId].total`." to explain it more thoroughly:

```
(which don't get updated in the allocation runs or during recovery of 
allocated resources)
```



src/tests/persistent_volume_tests.cpp (lines 1443 - 1447)


"We kill the long-lived task": this duplicates the same "We kill the 
long-lived task" below. I guess you mentioned them in two places because they 
are far apart but can we move 

```
  Future status4;
  EXPECT_CALL(sched, statusUpdate(, _))
.WillOnce(FutureArg<1>());
```

down to right before we sent killTask so we just need to comment on it once?

Plus, the block of code below it right now is all about killing the 
`nonSharedVolume` which is not relevant.


- Jiang Yan Xu


On Jan. 9, 2017, 2:37 p.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53096/
> ---
> 
> (Updated Jan. 9, 2017, 2:37 p.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6444
> https://issues.apache.org/jira/browse/MESOS-6444
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> We maintain a single copy of shared resource in the role and quota
> sorter's total resources. So, when we update these resources, we
> remove the previous resources at this agent and add the new resources
> at this agent.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.hpp 
> a6424d624864155e1c87a28a63b784512c5c8722 
>   src/master/allocator/mesos/hierarchical.cpp 
> 91b1ec43940a788459f045ca4a4b82d4e8373bca 
>   src/tests/persistent_volume_tests.cpp 
> 8198b6b5ad323d17835ba067c7ff3d34ef948125 
> 
> Diff: https://reviews.apache.org/r/53096/diff/
> 
> 
> Testing
> ---
> 
> Tests passed.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 53096: Fix handling in shared count in total resources in the sorter.

2017-01-25 Thread Jiang Yan Xu 


> On Dec. 18, 2016, 11:46 p.m., Jiang Yan Xu wrote:
> > src/master/allocator/mesos/hierarchical.hpp, lines 477-478
> > 
> >
> > Update the comment per the comment about the role of the method.
> > 
> > ```
> > // Helper to update the total resources in the allocator and each of 
> > the sorters.
> > ```
> 
> Anindya Sinha wrote:
> Not each of the sorters but only the role and quota sorters, since the 
> framework sorter's total is a subset of slave's total resources. Correct?

Ah you are right. However because of this it might make sense to name the 
method more explcitly, i.e., `updateSlaveTotal`, as in, frameworkSorter's total 
is not the agent's total? I'll comment further in the review.


- Jiang Yan


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


On Jan. 9, 2017, 2:37 p.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53096/
> ---
> 
> (Updated Jan. 9, 2017, 2:37 p.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6444
> https://issues.apache.org/jira/browse/MESOS-6444
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> We maintain a single copy of shared resource in the role and quota
> sorter's total resources. So, when we update these resources, we
> remove the previous resources at this agent and add the new resources
> at this agent.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.hpp 
> a6424d624864155e1c87a28a63b784512c5c8722 
>   src/master/allocator/mesos/hierarchical.cpp 
> 91b1ec43940a788459f045ca4a4b82d4e8373bca 
>   src/tests/persistent_volume_tests.cpp 
> 8198b6b5ad323d17835ba067c7ff3d34ef948125 
> 
> Diff: https://reviews.apache.org/r/53096/diff/
> 
> 
> Testing
> ---
> 
> Tests passed.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 53096: Fix handling in shared count in total resources in the sorter.

2017-01-09 Thread Anindya Sinha 

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

(Updated Jan. 9, 2017, 10:37 p.m.)


Review request for mesos and Jiang Yan Xu.


Changes
---

Rebase.


Bugs: MESOS-6444
https://issues.apache.org/jira/browse/MESOS-6444


Repository: mesos


Description
---

We maintain a single copy of shared resource in the role and quota
sorter's total resources. So, when we update these resources, we
remove the previous resources at this agent and add the new resources
at this agent.


Diffs (updated)
-

  src/master/allocator/mesos/hierarchical.hpp 
a6424d624864155e1c87a28a63b784512c5c8722 
  src/master/allocator/mesos/hierarchical.cpp 
91b1ec43940a788459f045ca4a4b82d4e8373bca 
  src/tests/persistent_volume_tests.cpp 
8198b6b5ad323d17835ba067c7ff3d34ef948125 

Diff: https://reviews.apache.org/r/53096/diff/


Testing
---

Tests passed.


Thanks,

Anindya Sinha

Re: Review Request 53096: Fix handling in shared count in total resources in the sorter.

2016-12-19 Thread Anindya Sinha 

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

(Updated Dec. 19, 2016, 7:24 p.m.)


Review request for mesos and Jiang Yan Xu.


Changes
---

Minor refactor and addressed review comments.


Bugs: MESOS-6444
https://issues.apache.org/jira/browse/MESOS-6444


Repository: mesos


Description
---

We maintain a single copy of shared resource in the role and quota
sorter's total resources. So, when we update these resources, we
remove the previous resources at this agent and add the new resources
at this agent.


Diffs (updated)
-

  src/master/allocator/mesos/hierarchical.hpp 
a6424d624864155e1c87a28a63b784512c5c8722 
  src/master/allocator/mesos/hierarchical.cpp 
91b1ec43940a788459f045ca4a4b82d4e8373bca 
  src/tests/persistent_volume_tests.cpp 
b7c313781f3dd9ab4fd68ce23ccc044d50b734d8 

Diff: https://reviews.apache.org/r/53096/diff/


Testing
---

Tests passed.


Thanks,

Anindya Sinha

Re: Review Request 53096: Fix handling in shared count in total resources in the sorter.

2016-12-19 Thread Anindya Sinha 


> On Dec. 19, 2016, 7:46 a.m., Jiang Yan Xu wrote:
> > src/master/allocator/mesos/hierarchical.hpp, lines 477-478
> > 
> >
> > Update the comment per the comment about the role of the method.
> > 
> > ```
> > // Helper to update the total resources in the allocator and each of 
> > the sorters.
> > ```

Not each of the sorters but only the role and quota sorters, since the 
framework sorter's total is a subset of slave's total resources. Correct?


> On Dec. 19, 2016, 7:46 a.m., Jiang Yan Xu wrote:
> > src/master/allocator/mesos/hierarchical.hpp, line 479
> > 
> >
> > 1. No need for the `_` prefix for private methods. We use it for 
> > continuations.
> > 2. `oldTotal`? Should be the updated total right? i.e.,
> > 
> > ```
> > void updateTotal(const SlaveID& slaveId, const Resources& total);
> > ```
> > 
> > I suggested just naming it `total` because I feel intuitively people 
> > would associate the `total` in a method called `updateTotal` to be "the 
> > value to update the totals to". What do you think?

Well, I was passing the `oldTotal` to be removed, and added 
`slaves[slaveId].total` (which was updated in the call site). Hence the name 
`oldTotal`. But I can pass in the new `total` (before updating) and remove 
`oldTotal` based on `slaves[slaveId].total` and then add new `total`.


> On Dec. 19, 2016, 7:46 a.m., Jiang Yan Xu wrote:
> > src/master/allocator/mesos/hierarchical.cpp, line 824
> > 
> >
> > Use the same comment:
> > 
> > ```
> > // Update the total resources in the allocator and each of the 
> > sorters.
> > ```

Not all sorters, only the role and quota sorters.


- Anindya


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


On Dec. 12, 2016, 10:35 p.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53096/
> ---
> 
> (Updated Dec. 12, 2016, 10:35 p.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6444
> https://issues.apache.org/jira/browse/MESOS-6444
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> We maintain a single copy of shared resource in the role and quota
> sorter's total resources. So, when we update these resources, we
> remove the previous resources at this agent and add the new resources
> at this agent.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.hpp 
> a6424d624864155e1c87a28a63b784512c5c8722 
>   src/master/allocator/mesos/hierarchical.cpp 
> a8cbc8db3d1319718765783827f8be1981b56f04 
>   src/tests/persistent_volume_tests.cpp 
> b7c313781f3dd9ab4fd68ce23ccc044d50b734d8 
> 
> Diff: https://reviews.apache.org/r/53096/diff/
> 
> 
> Testing
> ---
> 
> Tests passed.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 53096: Fix handling in shared count in total resources in the sorter.

2016-12-18 Thread Jiang Yan Xu 

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




src/master/allocator/mesos/hierarchical.hpp (lines 477 - 478)


Update the comment per the comment about the role of the method.

```
// Helper to update the total resources in the allocator and each of the 
sorters.
```



src/master/allocator/mesos/hierarchical.hpp (line 479)


1. No need for the `_` prefix for private methods. We use it for 
continuations.
2. `oldTotal`? Should be the updated total right? i.e.,

```
void updateTotal(const SlaveID& slaveId, const Resources& total);
```

I suggested just naming it `total` because I feel intuitively people would 
associate the `total` in a method called `updateTotal` to be "the value to 
update the totals to". What do you think?



src/master/allocator/mesos/hierarchical.cpp (lines 731 - 732)


Move to `updateTotal()`.



src/master/allocator/mesos/hierarchical.cpp (line 755)


Call the this way:

```
updateTotal(slaveId, updatedTotal);
```

And move this to right below `CHECK_SOME(updatedTotal);`.

Also we need to shuffle things around a bit so this block of code is 
cleanly splitted into two sections:

```
// Update the total resources in the allocator and each of the sorters.
Try updatedTotal = slaves[slaveId].total.apply(operation);
CHECK_SOME(updatedTotal);

updatedTotal(slaveId, updatedTotal);

// Update the allocated resources in the allocator and each of the 
sorters.
Try updatedSlaveAllocation =
  slaves[slaveId].allocated.apply(operation);

CHECK_SOME(updatedSlaveAllocation);

slaves[slaveId].allocated = updatedSlaveAllocation.get();

Resources frameworkAllocation =
  frameworkSorter->allocation(frameworkId.value(), slaveId);

...
```



src/master/allocator/mesos/hierarchical.cpp (lines 814 - 815)


Move over to `updateTotal()`.



src/master/allocator/mesos/hierarchical.cpp (line 817)


Use the same comment:

```
// Update the total resources in the allocator and each of the sorters.
```



src/master/allocator/mesos/hierarchical.cpp (line 1974)


s/oldTotal/total/



src/master/allocator/mesos/hierarchical.cpp (line 1977)


As commented above, add

```
const Resources oldTotal = slaves[slaveId].total;
slaves[slaveId].total = updatedTotal;
```



src/tests/persistent_volume_tests.cpp (lines 1373 - 1375)


Move the comments about killing the long-lived task below to where we are 
actually killing it?

Otherwise here you are describing for 4 statuses but are onlying defining 3.



src/tests/persistent_volume_tests.cpp (lines 1381 - 1383)


2 space indentation.



src/tests/persistent_volume_tests.cpp (line 1402)


Space after `foreach`.



src/tests/persistent_volume_tests.cpp (line 1432)


2 space indentation.



src/tests/persistent_volume_tests.cpp (line 1435)


2 space indentation.


- Jiang Yan Xu


On Dec. 12, 2016, 2:35 p.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53096/
> ---
> 
> (Updated Dec. 12, 2016, 2:35 p.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6444
> https://issues.apache.org/jira/browse/MESOS-6444
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> We maintain a single copy of shared resource in the role and quota
> sorter's total resources. So, when we update these resources, we
> remove the previous resources at this agent and add the new resources
> at this agent.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.hpp 
> a6424d624864155e1c87a28a63b784512c5c8722 
>   src/master/allocator/mesos/hierarchical.cpp 
> a8cbc8db3d1319718765783827f8be1981b56f04 
>   src/tests/persistent_volume_tests.cpp 
> b7c313781f3dd9ab4fd68ce23ccc044d50b734d8 
> 
> Diff:

Re: Review Request 53096: Fix handling in shared count in total resources in the sorter.

2016-12-12 Thread Anindya Sinha 

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

(Updated Dec. 12, 2016, 10:35 p.m.)


Review request for mesos and Jiang Yan Xu.


Changes
---

Rebased.


Bugs: MESOS-6444
https://issues.apache.org/jira/browse/MESOS-6444


Repository: mesos


Description
---

We maintain a single copy of shared resource in the role and quota
sorter's total resources. So, when we update these resources, we
remove the previous resources at this agent and add the new resources
at this agent.


Diffs (updated)
-

  src/master/allocator/mesos/hierarchical.hpp 
a6424d624864155e1c87a28a63b784512c5c8722 
  src/master/allocator/mesos/hierarchical.cpp 
a8cbc8db3d1319718765783827f8be1981b56f04 
  src/tests/persistent_volume_tests.cpp 
b7c313781f3dd9ab4fd68ce23ccc044d50b734d8 

Diff: https://reviews.apache.org/r/53096/diff/


Testing
---

Tests passed.


Thanks,

Anindya Sinha

Re: Review Request 53096: Fix handling in shared count in total resources in the sorter.

2016-12-12 Thread Anindya Sinha 

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

(Updated Dec. 12, 2016, 9:15 a.m.)


Review request for mesos and Jiang Yan Xu.


Changes
---

Addressed review comments, and rebased.


Bugs: MESOS-6444
https://issues.apache.org/jira/browse/MESOS-6444


Repository: mesos


Description
---

We maintain a single copy of shared resource in the role and quota
sorter's total resources. So, when we update these resources, we
remove the previous resources at this agent and add the new resources
at this agent.


Diffs (updated)
-

  src/master/allocator/mesos/hierarchical.hpp 
a6424d624864155e1c87a28a63b784512c5c8722 
  src/master/allocator/mesos/hierarchical.cpp 
a8cbc8db3d1319718765783827f8be1981b56f04 
  src/tests/persistent_volume_tests.cpp 
b7c313781f3dd9ab4fd68ce23ccc044d50b734d8 

Diff: https://reviews.apache.org/r/53096/diff/


Testing
---

Tests passed.


Thanks,

Anindya Sinha

Re: Review Request 53096: Fix handling in shared count in total resources in the sorter.

2016-12-11 Thread Jiang Yan Xu 

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




src/master/allocator/mesos/hierarchical.cpp (lines 722 - 727)


Can we have a private helper

```
void HierarchicalAllocatorProcess::updateTotal(
const SlaveID& slaveId,
const Resources& total)
{
...
}
```

and have `updateAllocation()` and `updateAvailable()` both call it? 
Obviously this reduces code duplication but I think more importantly we are 
able to comment in a single place about the connection between the role sorter 
and the quota role sorter's `total` and the "total resource size of the 
cluster" which is our rationale for this patch.

Something like:

```
Currently `roleSorter` and `quotaRoleSorter`, being the root-level sorters, 
maintain all of `slaves[slaveId].total` (or the `nonRevocable()` portion in the 
case of `quotaRoleSorter`) in their own totals. Therefore we update them using 
the same `updatedTotal` as used to update `slaves[slaveId].total`.
```



src/tests/persistent_volume_tests.cpp (lines 1281 - 1282)


The reason for this test, when not being looked at in the context of 
MESOS-6444, is a bit hard to understand. How about we add to the end of the 
comments:

```
This is to catch any regression to MESOS-6444.
```



src/tests/persistent_volume_tests.cpp (lines 1327 - 1329)


This doesn't help here and we should be fine without it.

If the agent registers first, when the scheduler connects it's going to get 
an offer without waiting for the next allocation cycle.

If the scheduler registers first, when the agent later registers, the 
master will generate an offer to the scheduler without waiting for the next 
allocation cycle either.



src/tests/persistent_volume_tests.cpp (line 1342)


Single space.



src/tests/persistent_volume_tests.cpp (line 1351)


s/long lived task/a long-lived task/



src/tests/persistent_volume_tests.cpp (line 1360)


s/short lived task/a short-lived task/



src/tests/persistent_volume_tests.cpp (line 1367)


Use "exit 0" as a "simplest dummy task" since it really doesn't matter what 
it does.



src/tests/persistent_volume_tests.cpp (lines 1382 - 1384)


Indentation. Also fix other occurrences in this review.



src/tests/persistent_volume_tests.cpp (lines 1383 - 1384)


There's no guarantee that TASK_FINISHED for task2 comes in after 
TASK_RUNNING for task1.

We could use the technique employed in /r/54134/.



src/tests/persistent_volume_tests.cpp (line 1446)


This is your long-lived task.



src/tests/persistent_volume_tests.cpp (lines 1447 - 1449)


It takes too long to wait for the long-lived task. It being 10 secs may not 
be enough for super slow VMs but may be too long for fast machines.

Can we make the long-lived task "sleep 1000" so it's really long-lived and 
then kill it explicitly here?



src/tests/persistent_volume_tests.cpp (line 1481)


s/SharedVolume/SharedPersistentVolume/

for a consistent naming pattern.



src/tests/persistent_volume_tests.cpp (line 1521)


s/1st/the 1st/



src/tests/persistent_volume_tests.cpp (line 1529)


Single space.



src/tests/persistent_volume_tests.cpp (line 1552)


"until" here reads a bit weird: we advance the clock once to trigger the 
allocator action; we are not keeping advancing the clock "until" the allocator 
action is triggered.

Maybe just:

```
// Advance the clock to generate an offer from the recovered resources.
```



src/tests/persistent_volume_tests.cpp (line 1563)


s/2nd/the 2nd/



src/tests/persistent_volume_tests.cpp (line 1571)


Single space.



src/tests/persistent_volume_tests.cpp (line 1589)


Ditto.



src/tests/persistent_volume_tests.cpp (lines 1594 - 1595)

Re: Review Request 53096: Fix handling in shared count in total resources in the sorter.

2016-12-02 Thread Anindya Sinha 

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

(Updated Dec. 2, 2016, 9:43 p.m.)


Review request for mesos and Jiang Yan Xu.


Changes
---

rebase.


Bugs: MESOS-6444
https://issues.apache.org/jira/browse/MESOS-6444


Repository: mesos


Description
---

We maintain a single copy of shared resource in the role and quota
sorter's total resources. So, when we update these resources, we
remove the previous resources at this agent and add the new resources
at this agent.


Diffs (updated)
-

  src/master/allocator/mesos/hierarchical.cpp 
3b759494071c4cae4b8b7dbcb0028df4146fc30e 
  src/tests/persistent_volume_tests.cpp 
b7c313781f3dd9ab4fd68ce23ccc044d50b734d8 

Diff: https://reviews.apache.org/r/53096/diff/


Testing
---

Tests passed.


Thanks,

Anindya Sinha

Re: Review Request 53096: Fix handling in shared count in total resources in the sorter.

2016-12-01 Thread Anindya Sinha 

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

(Updated Dec. 1, 2016, 10:37 p.m.)


Review request for mesos and Jiang Yan Xu.


Changes
---

rebased.


Bugs: MESOS-6444
https://issues.apache.org/jira/browse/MESOS-6444


Repository: mesos


Description
---

We maintain a single copy of shared resource in the role and quota
sorter's total resources. So, when we update these resources, we
remove the previous resources at this agent and add the new resources
at this agent.


Diffs (updated)
-

  src/master/allocator/mesos/hierarchical.cpp 
3b759494071c4cae4b8b7dbcb0028df4146fc30e 
  src/tests/persistent_volume_tests.cpp 
b7c313781f3dd9ab4fd68ce23ccc044d50b734d8 

Diff: https://reviews.apache.org/r/53096/diff/


Testing
---

Tests passed.


Thanks,

Anindya Sinha

Re: Review Request 53096: Fix handling in shared count in total resources in the sorter.

2016-11-28 Thread Anindya Sinha 


> On Nov. 19, 2016, 12:30 a.m., Jiang Yan Xu wrote:
> > src/master/allocator/mesos/hierarchical.cpp, line 757
> > 
> >
> > So if `operation` contians multiple copies, the result will end up 
> > having multiple copies right?

This piece of code is executed for non LAUNCH operations only. So we should 
have at most a single copy of shared resources at this point for a specific 
operation.


> On Nov. 19, 2016, 12:30 a.m., Jiang Yan Xu wrote:
> > src/master/allocator/mesos/hierarchical.cpp, lines 752-755
> > 
> >
> > This seems the wrong place to do it, multiple copies of the same shared 
> > resources are added to the allocation of the role (in roleSorter and 
> > quotaRoleSorter) from multiple places:
> > 
> > - updateAllocation
> > - addFramework
> > - addSlave
> > - allocate
> > 
> > If the rule is to not have more than one copy the shared resources to 
> > roleSorter and quotaRoleSorter, they should be invariants, i.e., we should 
> > prevent them from being added instead of deduping them here.
> > 
> > Let's chat about the design.

That is indeed the case. We can have multiple copies of shared resources in 
allocations for role sorter and quota sorter; but only a single copy of shared 
resources in the total maintained in role sorter and quota sorter. The issue 
here is that we use the shared count in allocations in framework sorter to 
remove appropriate resources in the role and quota sorter's total resources. 
Since total resources in role sorter and quota sorter is atmost 1, but 
allocations in framework sorter can be more which results in 
`CHECK(total_.resources[slaveId].contains(resources))` failing.

So, this fix seems to take care of the inconsistencies in the shared count. 
However, as discussed it would be better to use the agent's total resources 
(before and after applying the appropriate `operation`) to account for the 
total resources in the role and quota sorter, i.e. something similar to the 
approach in `HierarchicalAllocatorProcess::updateAvailable()`, as follows:

```
  // Update the total resources.
  Try updatedTotal = slaves[slaveId].total.apply(operations);
  CHECK_SOME(updatedTotal);

  const Resources oldTotal = slaves[slaveId].total;
  slaves[slaveId].total = updatedTotal.get();

  // Now, update the total resources in the role sorters by removing
  // the previous resources at this slave and adding the new resources.
  roleSorter->remove(slaveId, oldTotal);
  roleSorter->add(slaveId, updatedTotal.get());

  // See comment at `quotaRoleSorter` declaration regarding non-revocable.
  quotaRoleSorter->remove(slaveId, oldTotal.nonRevocable());
  quotaRoleSorter->add(slaveId, updatedTotal.get().nonRevocable());
```


- Anindya


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


On Nov. 18, 2016, 5:14 a.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53096/
> ---
> 
> (Updated Nov. 18, 2016, 5:14 a.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6444
> https://issues.apache.org/jira/browse/MESOS-6444
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> We maintain a single copy of shared resource in the role and quota
> sorter's total resources. So, when we update these resources, we ensure
> that we only count a single copy even though the framework sorter
> may be returned multiple copies of a shared resource.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.cpp 
> c8f9492ee1b69e125a1e841116d22a578a9b524e 
>   src/tests/persistent_volume_tests.cpp 
> 8651b2c5455089041f16d091c90a7e0d948191b8 
> 
> Diff: https://reviews.apache.org/r/53096/diff/
> 
> 
> Testing
> ---
> 
> Tests passed.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 53096: Fix handling in shared count in total resources in the sorter.

2016-11-28 Thread Anindya Sinha 

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

(Updated Nov. 29, 2016, 12:25 a.m.)


Review request for mesos and Jiang Yan Xu.


Changes
---

Addressed review comments.


Bugs: MESOS-6444
https://issues.apache.org/jira/browse/MESOS-6444


Repository: mesos


Description (updated)
---

We maintain a single copy of shared resource in the role and quota
sorter's total resources. So, when we update these resources, we
remove the previous resources at this agent and add the new resources
at this agent.


Diffs (updated)
-

  src/master/allocator/mesos/hierarchical.cpp 
3b759494071c4cae4b8b7dbcb0028df4146fc30e 
  src/tests/persistent_volume_tests.cpp 
8651b2c5455089041f16d091c90a7e0d948191b8 

Diff: https://reviews.apache.org/r/53096/diff/


Testing
---

Tests passed.


Thanks,

Anindya Sinha

Re: Review Request 53096: Fix handling in shared count in total resources in the sorter.

2016-11-18 Thread Jiang Yan Xu 

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




src/master/allocator/mesos/hierarchical.cpp (lines 752 - 755)


This seems the wrong place to do it, multiple copies of the same shared 
resources are added to the allocation of the role (in roleSorter and 
quotaRoleSorter) from multiple places:

- updateAllocation
- addFramework
- addSlave
- allocate

If the rule is to not have more than one copy the shared resources to 
roleSorter and quotaRoleSorter, they should be invariants, i.e., we should 
prevent them from being added instead of deduping them here.

Let's chat about the design.



src/master/allocator/mesos/hierarchical.cpp (line 757)


So if `operation` contians multiple copies, the result will end up having 
multiple copies right?


- Jiang Yan Xu


On Nov. 17, 2016, 9:14 p.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53096/
> ---
> 
> (Updated Nov. 17, 2016, 9:14 p.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6444
> https://issues.apache.org/jira/browse/MESOS-6444
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> We maintain a single copy of shared resource in the role and quota
> sorter's total resources. So, when we update these resources, we ensure
> that we only count a single copy even though the framework sorter
> may be returned multiple copies of a shared resource.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.cpp 
> c8f9492ee1b69e125a1e841116d22a578a9b524e 
>   src/tests/persistent_volume_tests.cpp 
> 8651b2c5455089041f16d091c90a7e0d948191b8 
> 
> Diff: https://reviews.apache.org/r/53096/diff/
> 
> 
> Testing
> ---
> 
> Tests passed.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 53096: Fix handling in shared count in total resources in the sorter.

2016-11-17 Thread Anindya Sinha 

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

(Updated Nov. 18, 2016, 5:14 a.m.)


Review request for mesos and Jiang Yan Xu.


Changes
---

rebased.


Bugs: MESOS-6444
https://issues.apache.org/jira/browse/MESOS-6444


Repository: mesos


Description
---

We maintain a single copy of shared resource in the role and quota
sorter's total resources. So, when we update these resources, we ensure
that we only count a single copy even though the framework sorter
may be returned multiple copies of a shared resource.


Diffs (updated)
-

  src/master/allocator/mesos/hierarchical.cpp 
c8f9492ee1b69e125a1e841116d22a578a9b524e 
  src/tests/persistent_volume_tests.cpp 
8651b2c5455089041f16d091c90a7e0d948191b8 

Diff: https://reviews.apache.org/r/53096/diff/


Testing
---

Tests passed.


Thanks,

Anindya Sinha

Re: Review Request 53096: Fix handling in shared count in total resources in the sorter.

2016-11-17 Thread Anindya Sinha 

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

(Updated Nov. 18, 2016, 4:32 a.m.)


Review request for mesos and Jiang Yan Xu.


Changes
---

rebased.


Bugs: MESOS-6444
https://issues.apache.org/jira/browse/MESOS-6444


Repository: mesos


Description
---

We maintain a single copy of shared resource in the role and quota
sorter's total resources. So, when we update these resources, we ensure
that we only count a single copy even though the framework sorter
may be returned multiple copies of a shared resource.


Diffs (updated)
-

  src/master/allocator/mesos/hierarchical.cpp 
c8f9492ee1b69e125a1e841116d22a578a9b524e 
  src/tests/persistent_volume_tests.cpp 
8651b2c5455089041f16d091c90a7e0d948191b8 

Diff: https://reviews.apache.org/r/53096/diff/


Testing
---

Tests passed.


Thanks,

Anindya Sinha

Re: Review Request 53096: Fix handling in shared count in total resources in the sorter.

2016-11-07 Thread Anindya Sinha 

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

(Updated Nov. 8, 2016, 5:50 a.m.)


Review request for mesos and Jiang Yan Xu.


Changes
---

Rebased


Bugs: MESOS-6444
https://issues.apache.org/jira/browse/MESOS-6444


Repository: mesos


Description
---

We maintain a single copy of shared resource in the role and quota
sorter's total resources. So, when we update these resources, we ensure
that we only count a single copy even though the framework sorter
may be returned multiple copies of a shared resource.


Diffs (updated)
-

  src/master/allocator/mesos/hierarchical.cpp 
c8f9492ee1b69e125a1e841116d22a578a9b524e 
  src/tests/persistent_volume_tests.cpp 
8651b2c5455089041f16d091c90a7e0d948191b8 

Diff: https://reviews.apache.org/r/53096/diff/


Testing
---

Tests passed.


Thanks,

Anindya Sinha

Review Request 53096: Fix handling in shared count in total resources in the sorter.

2016-10-21 Thread Anindya Sinha 

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

Review request for mesos and Jiang Yan Xu.


Bugs: MESOS-6444
https://issues.apache.org/jira/browse/MESOS-6444


Repository: mesos


Description
---

We maintain a single copy of shared resource in the role and quota
sorter's total resources. So, when we update these resources, we ensure
that we only count a single copy even though the framework sorter
may be returned multiple copies of a shared resource.


Diffs
-

  src/master/allocator/mesos/hierarchical.cpp 
c8f9492ee1b69e125a1e841116d22a578a9b524e 
  src/tests/persistent_volume_tests.cpp 
6289009fe9ed0a57ba5eff46dbbe0633a75d2616 

Diff: https://reviews.apache.org/r/53096/diff/


Testing
---

Tests passed.


Thanks,

Anindya Sinha