Re: Review Request 52295: Added additional unit tests for shared resources.

2016-12-01 Thread Jiang Yan Xu 

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


Ship it!




Will commit after I run it.


src/tests/sorter_tests.cpp (line 747)


This should replace line 740?


- Jiang Yan Xu


On Nov. 18, 2016, 11:49 a.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52295/
> ---
> 
> (Updated Nov. 18, 2016, 11:49 a.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6185
> https://issues.apache.org/jira/browse/MESOS-6185
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Tests include the following:
> - Allocator tests when dealing with multiple frameworks of the same
>   role using the same shared volume at the same time.
> - Validity of DESTROY shared volume when there are pending tasks
>   that have been assigned that shared volume.
> - Handlng of master failover.
> - Sorter tests covering shared resources.
> 
> 
> Diffs
> -
> 
>   src/tests/hierarchical_allocator_tests.cpp 
> edd5cea8996d7c616cf9428f0f1c05d70c37c307 
>   src/tests/master_validation_tests.cpp 
> f893067859425967654401f3226149268b51cf57 
>   src/tests/persistent_volume_tests.cpp 
> 8651b2c5455089041f16d091c90a7e0d948191b8 
>   src/tests/sorter_tests.cpp 9f48abe65c011acfaf79f3d0d79f1d032fd6a4af 
> 
> Diff: https://reviews.apache.org/r/52295/diff/
> 
> 
> Testing
> ---
> 
> All tests passed.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 52295: Added additional unit tests for shared resources.

2016-11-18 Thread Mesos ReviewBot 

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



Patch looks great!

Reviews applied: [52295]

Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' 
CONFIGURATION='--verbose' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; 
./support/docker_build.sh

- Mesos ReviewBot


On Nov. 18, 2016, 7:49 p.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52295/
> ---
> 
> (Updated Nov. 18, 2016, 7:49 p.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6185
> https://issues.apache.org/jira/browse/MESOS-6185
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Tests include the following:
> - Allocator tests when dealing with multiple frameworks of the same
>   role using the same shared volume at the same time.
> - Validity of DESTROY shared volume when there are pending tasks
>   that have been assigned that shared volume.
> - Handlng of master failover.
> - Sorter tests covering shared resources.
> 
> 
> Diffs
> -
> 
>   src/tests/hierarchical_allocator_tests.cpp 
> edd5cea8996d7c616cf9428f0f1c05d70c37c307 
>   src/tests/master_validation_tests.cpp 
> f893067859425967654401f3226149268b51cf57 
>   src/tests/persistent_volume_tests.cpp 
> 8651b2c5455089041f16d091c90a7e0d948191b8 
>   src/tests/sorter_tests.cpp 9f48abe65c011acfaf79f3d0d79f1d032fd6a4af 
> 
> Diff: https://reviews.apache.org/r/52295/diff/
> 
> 
> Testing
> ---
> 
> All tests passed.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 52295: Added additional unit tests for shared resources.

2016-11-18 Thread Anindya Sinha 

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

(Updated Nov. 18, 2016, 7:49 p.m.)


Review request for mesos and Jiang Yan Xu.


Changes
---

A minor cosmetic change.


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


Repository: mesos


Description
---

Tests include the following:
- Allocator tests when dealing with multiple frameworks of the same
  role using the same shared volume at the same time.
- Validity of DESTROY shared volume when there are pending tasks
  that have been assigned that shared volume.
- Handlng of master failover.
- Sorter tests covering shared resources.


Diffs (updated)
-

  src/tests/hierarchical_allocator_tests.cpp 
edd5cea8996d7c616cf9428f0f1c05d70c37c307 
  src/tests/master_validation_tests.cpp 
f893067859425967654401f3226149268b51cf57 
  src/tests/persistent_volume_tests.cpp 
8651b2c5455089041f16d091c90a7e0d948191b8 
  src/tests/sorter_tests.cpp 9f48abe65c011acfaf79f3d0d79f1d032fd6a4af 

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


Testing
---

All tests passed.


Thanks,

Anindya Sinha

Re: Review Request 52295: Added additional unit tests for shared resources.

2016-11-18 Thread Anindya Sinha 

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

(Updated Nov. 18, 2016, 7:48 p.m.)


Review request for mesos and Jiang Yan Xu.


Changes
---

Addressed review comments.


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


Repository: mesos


Description
---

Tests include the following:
- Allocator tests when dealing with multiple frameworks of the same
  role using the same shared volume at the same time.
- Validity of DESTROY shared volume when there are pending tasks
  that have been assigned that shared volume.
- Handlng of master failover.
- Sorter tests covering shared resources.


Diffs (updated)
-

  src/tests/hierarchical_allocator_tests.cpp 
edd5cea8996d7c616cf9428f0f1c05d70c37c307 
  src/tests/master_validation_tests.cpp 
f893067859425967654401f3226149268b51cf57 
  src/tests/persistent_volume_tests.cpp 
8651b2c5455089041f16d091c90a7e0d948191b8 
  src/tests/sorter_tests.cpp 9f48abe65c011acfaf79f3d0d79f1d032fd6a4af 

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


Testing
---

All tests passed.


Thanks,

Anindya Sinha

Re: Review Request 52295: Added additional unit tests for shared resources.

2016-11-18 Thread Jiang Yan Xu 

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


Fix it, then Ship it!





src/tests/hierarchical_allocator_tests.cpp (line 3532)



s/HierarchicalAllocatorCommonRoleQuotaTest/HierarchicalAllocatorTesWithParam/

Yeah it's hard to pick a concise yet descriptive name about the role and 
the quota, etc. so just leave it to the comments to explain it?



src/tests/hierarchical_allocator_tests.cpp (lines 3537 - 3538)


Add a TODO above:

```
Move over more allocator tests that make sense to run both when the role is 
quota'ed and not. 
```



src/tests/hierarchical_allocator_tests.cpp (line 3540)


s/MultiFrameworks/QuotaSwitch/

as in, the Bool() is a switch to turn quota on and off?



src/tests/hierarchical_allocator_tests.cpp (line 3542)


s/Values(false, true)/Bool()/



src/tests/hierarchical_allocator_tests.cpp (line 3546)


s/it is/they are/



src/tests/hierarchical_allocator_tests.cpp (lines 3547 - 3548)


The correct indentation would be:

```
TEST_P(HierarchicalAllocatorTesWithParam,
   AllocateSharedResources)
{
}
```

"MultiFrameworks" doesn't feel like it's serving a qualifying purpose here 
(we don't have another test for a single framework). I'd say we just use the 
shorter test name "AllocateSharedResources".



src/tests/hierarchical_allocator_tests.cpp (line 3595)


s/update/updated/ a better name?



src/tests/hierarchical_allocator_tests.cpp (lines 3609 - 3611)


Not clear to me what this means...

How about 

```
Note that the volume is not recovered as it's used by the task (but it's 
still offerable because it's shared).
```



src/tests/master_validation_tests.cpp (line 696)


No need for this variable, just use `sharedDisk` below?



src/tests/master_validation_tests.cpp (line 702)


s/""/"sleep 1000"/



src/tests/master_validation_tests.cpp (line 710)


You should be able to do:

```
pendingTasks[frameworkId1] = {{task.task_id(), task}};
```

and thus don't need the single use variable `taskMap`.



src/tests/master_validation_tests.cpp (line 718)


Remove a redundant space.



src/tests/persistent_volume_tests.cpp (line 1199)


One space before `//`.



src/tests/persistent_volume_tests.cpp (line 1203)


This line no longer relevant?



src/tests/sorter_tests.cpp (line 580)


s/likely hood/likelyhood/



src/tests/sorter_tests.cpp (line 706)


No need to wrap `sharedDisk` with `Resources()`?



src/tests/sorter_tests.cpp (line 732)


The meaning of `totalResources` here is not very clear to me: if should 
have disk of 1000 but here it's 900. Later the `shareDisk` is added to sorter 
and not to this variable.

Just simply:

```
  sorter.add(
  slaveId,
  Resources::parse("cpus:100;mem:100;disk(role1):900").get());
```
?



src/tests/sorter_tests.cpp (line 736)


s/origTotalScalarQuantity/quantity1/ ?

We are applying a sequence of operatoins to the sorter. Names like 
quantity1, quantity2 and quantity3 are as clear as these long names?



src/tests/sorter_tests.cpp (line 738)


No need to wrap it with `Resources()`?

Here and elsewhere.



src/tests/sorter_tests.cpp (line 741)


Not just "NE", we expect that

```
EXPECT_EQ(Resources::parse("disk(role1):100").get(), quantity2 - quantity1);
```

right?



src/tests/sorter_tests.cpp (line 749)


"it doesn't get removed from the sorter"

I am being pedantic but the copy does get removed, just not the quantity. 
How about

```
// The quantity of the shared disk is removed only when the last copy is

Re: Review Request 52295: Added additional unit tests for shared resources.

2016-11-11 Thread Anindya Sinha 


> On Nov. 10, 2016, 5:15 p.m., Jiang Yan Xu wrote:
> > src/tests/hierarchical_allocator_tests.cpp, line 2158
> > 
> >
> > This test is basically identitcal to 
> > 'AllocateSharedResourcesMultiFrameworks', can we templatize it?

Moved the two tests into a single parameterized test 
(HierarchicalAllocatorCommonRoleQuotaTest.AllocateSharedResourcesMultiFrameworks).


> On Nov. 10, 2016, 5:15 p.m., Jiang Yan Xu wrote:
> > src/tests/persistent_volume_tests.cpp, line 1011
> > 
> >
> > With the new test helper we now use:
> > 
> > ```
> > Owned detector = master.get()->createDetector();
> > Try slave = StartSlave(detector.get(), 
> > slaveFlags);
> > ```

Since this is a test for master failover, we need to point the detector to the 
new master, which is accomplished using `StandaloneMasterDetector::appoint()`, 
which cannot be achieved using `MasterDetector`. So, I kept this test as is.


- Anindya


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


On Sept. 27, 2016, 12:39 a.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52295/
> ---
> 
> (Updated Sept. 27, 2016, 12:39 a.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6185
> https://issues.apache.org/jira/browse/MESOS-6185
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Tests include the following:
> - Allocator tests when dealing with multiple frameworks of the same
>   role using the same shared volume at the same time.
> - Validity of DESTROY shared volume when there are pending tasks
>   that have been assigned that shared volume.
> - Handlng of master failover.
> - Sorter tests covering shared resources.
> 
> 
> Diffs
> -
> 
>   src/tests/hierarchical_allocator_tests.cpp 
> 46553f6d501deb37862dd5ba48be1c114f6e6cb8 
>   src/tests/master_validation_tests.cpp 
> 16c5773aa44016f923e00cb348ded6b8c46d4b4b 
>   src/tests/persistent_volume_tests.cpp 
> 2ab44d11d159162dfcac9d0791b651ed059b8164 
>   src/tests/sorter_tests.cpp 1f17c011898836ea9159661dde7d544cb0d8ea83 
> 
> Diff: https://reviews.apache.org/r/52295/diff/
> 
> 
> Testing
> ---
> 
> All tests passed.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 52295: Added additional unit tests for shared resources.

2016-11-11 Thread Anindya Sinha 

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

(Updated Nov. 11, 2016, 6:56 p.m.)


Review request for mesos and Jiang Yan Xu.


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


Repository: mesos


Description
---

Tests include the following:
- Allocator tests when dealing with multiple frameworks of the same
  role using the same shared volume at the same time.
- Validity of DESTROY shared volume when there are pending tasks
  that have been assigned that shared volume.
- Handlng of master failover.
- Sorter tests covering shared resources.


Diffs (updated)
-

  src/tests/hierarchical_allocator_tests.cpp 
edd5cea8996d7c616cf9428f0f1c05d70c37c307 
  src/tests/master_validation_tests.cpp 
7f7bb675b51370036be8edb22ab6cb5f8f913146 
  src/tests/persistent_volume_tests.cpp 
8651b2c5455089041f16d091c90a7e0d948191b8 
  src/tests/sorter_tests.cpp 9f48abe65c011acfaf79f3d0d79f1d032fd6a4af 

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


Testing
---

All tests passed.


Thanks,

Anindya Sinha

Re: Review Request 52295: Added additional unit tests for shared resources.

2016-11-10 Thread Jiang Yan Xu 

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




src/tests/hierarchical_allocator_tests.cpp (lines 1512 - 1513)


The way this test is written, it's not testing special properties of shared 
persistent volumes at all. Persistent volumes have the same behavior...

Shared persistent volumes are offered one-by-one as well but are "offerable 
even if it's already in use". We should test that instead.



src/tests/hierarchical_allocator_tests.cpp (line 1519)


s/with opt/opted in/?



src/tests/hierarchical_allocator_tests.cpp (lines 1530 - 1533)


Is 3 better than 2 in this case? Can we shorten this test?



src/tests/hierarchical_allocator_tests.cpp (lines 1580 - 1581)


We don't use this style. Instead:

```
  EXPECT_EQ(
  allocation.get().resources.get(slave.id()).get().shared(),
  Resources(volume));
```



src/tests/hierarchical_allocator_tests.cpp (line 2158)


This test is basically identitcal to 
'AllocateSharedResourcesMultiFrameworks', can we templatize it?



src/tests/master_validation_tests.cpp (lines 674 - 681)


Use `createTask()`.



src/tests/master_validation_tests.cpp (lines 683 - 690)


The existence of `task2` (and `disk2` for that matter) doesn't seem 
necessary?

They use disctint resources and can't imagine them interfere with each 
other?

Shorten the test?



src/tests/persistent_volume_tests.cpp (lines 1002 - 1003)


Can we add how we verify "the master recovers"? e.g.,

```
// This test verifies that the master recovers after a failover and 
re-offers the shared persistent volume when tasks using the same volume are 
still running.
```



src/tests/persistent_volume_tests.cpp (line 1004)


s/SharedVolume/SharedPersistentVolume/

Full spelling helps with code grepping.



src/tests/persistent_volume_tests.cpp (lines 1006 - 1008)


No `masterFlags` necessary.



src/tests/persistent_volume_tests.cpp (line 1011)


With the new test helper we now use:

```
Owned detector = master.get()->createDetector();
Try slave = StartSlave(detector.get(), slaveFlags);
```



src/tests/persistent_volume_tests.cpp (lines 1050 - 1065)


Intead of actually writing files, we should probably keep them up via 
"sleep 1000" so they are running during the failover.

BTW we need to have expectations for status updates otherwise there are 
going to be warnings?



src/tests/persistent_volume_tests.cpp (lines 1089 - 1090)


Nit: The relevant slave behavior can be inferred from the offer so we don't 
need this expectation.



src/tests/sorter_tests.cpp (line 637)


s/disk 50/disk of 50/



src/tests/sorter_tests.cpp (lines 660 - 661)


This actually fits one line? If not, the indentation should be

```
  Resources additionalAShared =
Resources(sharedDisk) + sharedDisk + sharedDisk;
```



src/tests/sorter_tests.cpp (line 705)


s/the fair/its dominant/



src/tests/sorter_tests.cpp (line 722)


For this test, I think a simple implementation would be to `add()` the same 
resources twice and `remove()` it twice. The `totalScalarQuantities()` only 
drops after the second `remove()`.



src/tests/sorter_tests.cpp (lines 733 - 738)


Mesos wouldn't add the same shared resources in this way. Call 
`sorter->add()` multiple times instead?


- Jiang Yan Xu


On Sept. 26, 2016, 5:39 p.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52295/
> ---
> 
> (Updated Sept. 26, 2016, 5:39 p.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6185
> https://issues.apache.org/jira/browse/MESOS-6185
> 
> 
> Repository: mesos
> 
>

Re: Review Request 52295: Added additional unit tests for shared resources.

2016-09-27 Thread Mesos ReviewBot 

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



Patch looks great!

Reviews applied: [52295]

Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' 
CONFIGURATION='--verbose' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; 
./support/docker_build.sh

- Mesos ReviewBot


On Sept. 27, 2016, 12:39 a.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52295/
> ---
> 
> (Updated Sept. 27, 2016, 12:39 a.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6185
> https://issues.apache.org/jira/browse/MESOS-6185
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Tests include the following:
> - Allocator tests when dealing with multiple frameworks of the same
>   role using the same shared volume at the same time.
> - Validity of DESTROY shared volume when there are pending tasks
>   that have been assigned that shared volume.
> - Handlng of master failover.
> - Sorter tests covering shared resources.
> 
> 
> Diffs
> -
> 
>   src/tests/hierarchical_allocator_tests.cpp 
> 46553f6d501deb37862dd5ba48be1c114f6e6cb8 
>   src/tests/master_validation_tests.cpp 
> 16c5773aa44016f923e00cb348ded6b8c46d4b4b 
>   src/tests/persistent_volume_tests.cpp 
> 2ab44d11d159162dfcac9d0791b651ed059b8164 
>   src/tests/sorter_tests.cpp 1f17c011898836ea9159661dde7d544cb0d8ea83 
> 
> Diff: https://reviews.apache.org/r/52295/diff/
> 
> 
> Testing
> ---
> 
> All tests passed.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Review Request 52295: Added additional unit tests for shared resources.

2016-09-26 Thread Anindya Sinha 

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

Review request for mesos and Jiang Yan Xu.


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


Repository: mesos


Description
---

Tests include the following:
- Allocator tests when dealing with multiple frameworks of the same
  role using the same shared volume at the same time.
- Validity of DESTROY shared volume when there are pending tasks
  that have been assigned that shared volume.
- Handlng of master failover.
- Sorter tests covering shared resources.


Diffs
-

  src/tests/hierarchical_allocator_tests.cpp 
46553f6d501deb37862dd5ba48be1c114f6e6cb8 
  src/tests/master_validation_tests.cpp 
16c5773aa44016f923e00cb348ded6b8c46d4b4b 
  src/tests/persistent_volume_tests.cpp 
2ab44d11d159162dfcac9d0791b651ed059b8164 
  src/tests/sorter_tests.cpp 1f17c011898836ea9159661dde7d544cb0d8ea83 

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


Testing
---

All tests passed.


Thanks,

Anindya Sinha