Re: Review Request 45962: Updated a persistent volume test framework to include shared volumes.

[Anindya Sinha]

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

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


Review request for mesos, Greg Mann, Jie Yu, and Jiang Yan Xu.


Changes
---

Refactor.


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


Repository: mesos


Description
---

Updated a persistent volume test framework to include shared volumes.


Diffs (updated)
-

  src/examples/persistent_volume_framework.cpp 
9d45bb496c4cf378af429b5aa970bf81450e482a 

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


Testing
---

New test framework for shared resources added.
Tests successful.


Thanks,

Anindya Sinha

Re: Review Request 45962: Updated a persistent volume test framework to include shared volumes.

[Anindya Sinha]

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

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


Review request for mesos, Greg Mann, Jie Yu, and Jiang Yan Xu.


Changes
---

Rebased after batch allocation patch was merged.


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


Repository: mesos


Description
---

Updated a persistent volume test framework to include shared volumes.


Diffs (updated)
-

  src/examples/persistent_volume_framework.cpp 
9d45bb496c4cf378af429b5aa970bf81450e482a 

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


Testing
---

New test framework for shared resources added.
Tests successful.


Thanks,

Anindya Sinha

Re: Review Request 45962: Updated a persistent volume test framework to include shared volumes.

[Anindya Sinha]

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

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


Review request for mesos, Greg Mann, Jie Yu, and Jiang Yan Xu.


Changes
---

Rebased.


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


Repository: mesos


Description
---

Updated a persistent volume test framework to include shared volumes.


Diffs (updated)
-

  src/examples/persistent_volume_framework.cpp 
9d45bb496c4cf378af429b5aa970bf81450e482a 

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


Testing
---

New test framework for shared resources added.
Tests successful.


Thanks,

Anindya Sinha

Re: Review Request 45962: Updated a persistent volume test framework to include shared volumes.

[Anindya Sinha]

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

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


Review request for mesos, Greg Mann, Jie Yu, and Jiang Yan Xu.


Changes
---

Addressed review comments.


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


Repository: mesos


Description
---

Updated a persistent volume test framework to include shared volumes.


Diffs (updated)
-

  src/examples/persistent_volume_framework.cpp 
9d45bb496c4cf378af429b5aa970bf81450e482a 

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


Testing
---

New test framework for shared resources added.
Tests successful.


Thanks,

Anindya Sinha

Re: Review Request 45962: Updated a persistent volume test framework to include shared volumes.

[Jiang Yan Xu]

> On Dec. 19, 2016, 12:01 a.m., Jiang Yan Xu wrote:
> > src/examples/persistent_volume_framework.cpp, lines 244-248
> > 
> >
> > Good find. However I think the fix would be cleaner if we just update 
> > the state of the shard to `Shard::STAGING` in this block code (in 
> > `resourceOffers()`), after the shard has launched all the required tasks.
> > 
> > Note that this is consistent with the definition of the state:
> > 
> > ```
> > RUNNING, // The shard is running (i.e., all tasks have launched).
> > ```
> > 
> > The definition suggests we should update the state after "all tasks 
> > have launched". In this framework we assume all launched tasks are going to 
> > successful run.
> 
> Anindya Sinha wrote:
> I think you meant `Shard::RUNNING`.
> I think changing state to `Shard::RUNNING` (and `Shard::TERMINATING`) is 
> better in a single place, i.e. when the task status updates is received. I 
> understand this is a test framework when we expect all launched tasks to be 
> successful but IMO, making state changes when we are sure of the event 
> (successfully launching or terminating a task) is preferable.
> 
> I would prefer to restate `Shard::RUNNING` as:
> ```
> RUNNING, // The shard is running (i.e., all tasks have successfully 
> launched). 
> ```

Alright then. But then we'd better adjust the definition of `Shard::RUNNING`. :)


- Jiang Yan


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


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/45962/
> ---
> 
> (Updated Jan. 9, 2017, 2:37 p.m.)
> 
> 
> Review request for mesos, Greg Mann, Jie Yu, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-4431
> https://issues.apache.org/jira/browse/MESOS-4431
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated a persistent volume test framework to include shared volumes.
> 
> 
> Diffs
> -
> 
>   src/examples/persistent_volume_framework.cpp 
> 9d45bb496c4cf378af429b5aa970bf81450e482a 
> 
> Diff: https://reviews.apache.org/r/45962/diff/
> 
> 
> Testing
> ---
> 
> New test framework for shared resources added.
> Tests successful.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 45962: Updated a persistent volume test framework to include shared volumes.

[Jiang Yan Xu]

> On Jan. 25, 2017, 5:27 p.m., Jiang Yan Xu wrote:
> > Just a minor comment plus the reply to a previous comment.

This can go in before /r/55359/ since that is a refactor.


- Jiang Yan


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


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/45962/
> ---
> 
> (Updated Jan. 9, 2017, 2:37 p.m.)
> 
> 
> Review request for mesos, Greg Mann, Jie Yu, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-4431
> https://issues.apache.org/jira/browse/MESOS-4431
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated a persistent volume test framework to include shared volumes.
> 
> 
> Diffs
> -
> 
>   src/examples/persistent_volume_framework.cpp 
> 9d45bb496c4cf378af429b5aa970bf81450e482a 
> 
> Diff: https://reviews.apache.org/r/45962/diff/
> 
> 
> Testing
> ---
> 
> New test framework for shared resources added.
> Tests successful.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 45962: Updated a persistent volume test framework to include shared volumes.

[Jiang Yan Xu]

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


Fix it, then Ship it!




Just a minor comment plus the reply to a previous comment.


src/examples/persistent_volume_framework.cpp (lines 248 - 249)


This 

```
} else {
  if (...) {
  }
}
```

looks odd. 

You could have done `else if` but in this particular case I guess `break` 
sonner is better? (A simple condition check + keep the bulk of the logic less 
indented).


- 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/45962/
> ---
> 
> (Updated Jan. 9, 2017, 2:37 p.m.)
> 
> 
> Review request for mesos, Greg Mann, Jie Yu, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-4431
> https://issues.apache.org/jira/browse/MESOS-4431
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated a persistent volume test framework to include shared volumes.
> 
> 
> Diffs
> -
> 
>   src/examples/persistent_volume_framework.cpp 
> 9d45bb496c4cf378af429b5aa970bf81450e482a 
> 
> Diff: https://reviews.apache.org/r/45962/diff/
> 
> 
> Testing
> ---
> 
> New test framework for shared resources added.
> Tests successful.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 45962: Updated a persistent volume test framework to include shared volumes.

[Anindya Sinha]

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

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


Review request for mesos, Greg Mann, Jie Yu, and Jiang Yan Xu.


Changes
---

Rebase.


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


Repository: mesos


Description
---

Updated a persistent volume test framework to include shared volumes.


Diffs (updated)
-

  src/examples/persistent_volume_framework.cpp 
9d45bb496c4cf378af429b5aa970bf81450e482a 

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


Testing
---

New test framework for shared resources added.
Tests successful.


Thanks,

Anindya Sinha

Re: Review Request 45962: Updated a persistent volume test framework to include shared volumes.

[Anindya Sinha]

> On Dec. 19, 2016, 8:01 a.m., Jiang Yan Xu wrote:
> > src/examples/persistent_volume_framework.cpp, lines 244-248
> > 
> >
> > Good find. However I think the fix would be cleaner if we just update 
> > the state of the shard to `Shard::STAGING` in this block code (in 
> > `resourceOffers()`), after the shard has launched all the required tasks.
> > 
> > Note that this is consistent with the definition of the state:
> > 
> > ```
> > RUNNING, // The shard is running (i.e., all tasks have launched).
> > ```
> > 
> > The definition suggests we should update the state after "all tasks 
> > have launched". In this framework we assume all launched tasks are going to 
> > successful run.

I think you meant `Shard::RUNNING`.
I think changing state to `Shard::RUNNING` (and `Shard::TERMINATING`) is better 
in a single place, i.e. when the task status updates is received. I understand 
this is a test framework when we expect all launched tasks to be successful but 
IMO, making state changes when we are sure of the event (successfully launching 
or terminating a task) is preferable.

I would prefer to restate `Shard::RUNNING` as:
```
RUNNING, // The shard is running (i.e., all tasks have successfully 
launched). 
```


- Anindya


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


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/45962/
> ---
> 
> (Updated Dec. 12, 2016, 10:35 p.m.)
> 
> 
> Review request for mesos, Greg Mann, Jie Yu, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-4431
> https://issues.apache.org/jira/browse/MESOS-4431
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated a persistent volume test framework to include shared volumes.
> 
> 
> Diffs
> -
> 
>   src/examples/persistent_volume_framework.cpp 
> 9d45bb496c4cf378af429b5aa970bf81450e482a 
> 
> Diff: https://reviews.apache.org/r/45962/diff/
> 
> 
> Testing
> ---
> 
> New test framework for shared resources added.
> Tests successful.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 45962: Updated a persistent volume test framework to include shared volumes.

[Anindya Sinha]

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

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


Review request for mesos, Greg Mann, Jie Yu, and Jiang Yan Xu.


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


Repository: mesos


Description
---

Updated a persistent volume test framework to include shared volumes.


Diffs (updated)
-

  src/examples/persistent_volume_framework.cpp 
9d45bb496c4cf378af429b5aa970bf81450e482a 

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


Testing
---

New test framework for shared resources added.
Tests successful.


Thanks,

Anindya Sinha

Re: Review Request 45962: Updated a persistent volume test framework to include shared volumes.

[Jiang Yan Xu]

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


Fix it, then Ship it!




Otherwise it looks good. I am just going to commit it together with /r/53096/.


src/examples/persistent_volume_framework.cpp (lines 244 - 248)


Good find. However I think the fix would be cleaner if we just update the 
state of the shard to `Shard::STAGING` in this block code (in 
`resourceOffers()`), after the shard has launched all the required tasks.

Note that this is consistent with the definition of the state:

```
RUNNING, // The shard is running (i.e., all tasks have launched).
```

The definition suggests we should update the state after "all tasks have 
launched". In this framework we assume all launched tasks are going to 
successful run.


- 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/45962/
> ---
> 
> (Updated Dec. 12, 2016, 2:35 p.m.)
> 
> 
> Review request for mesos, Greg Mann, Jie Yu, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-4431
> https://issues.apache.org/jira/browse/MESOS-4431
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated a persistent volume test framework to include shared volumes.
> 
> 
> Diffs
> -
> 
>   src/examples/persistent_volume_framework.cpp 
> 9d45bb496c4cf378af429b5aa970bf81450e482a 
> 
> Diff: https://reviews.apache.org/r/45962/diff/
> 
> 
> Testing
> ---
> 
> New test framework for shared resources added.
> Tests successful.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 45962: Updated a persistent volume test framework to include shared volumes.

[Anindya Sinha]

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

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


Review request for mesos, Greg Mann, Jie Yu, and Jiang Yan Xu.


Changes
---

Fixed a race condition.


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


Repository: mesos


Description
---

Updated a persistent volume test framework to include shared volumes.


Diffs (updated)
-

  src/examples/persistent_volume_framework.cpp 
9d45bb496c4cf378af429b5aa970bf81450e482a 

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


Testing
---

New test framework for shared resources added.
Tests successful.


Thanks,

Anindya Sinha

Re: Review Request 45962: Updated a persistent volume test framework to include shared volumes.

[Anindya Sinha]

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

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


Review request for mesos, Greg Mann, Jie Yu, and Jiang Yan Xu.


Changes
---

Rebased.


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


Repository: mesos


Description
---

Updated a persistent volume test framework to include shared volumes.


Diffs (updated)
-

  src/examples/persistent_volume_framework.cpp 
9d45bb496c4cf378af429b5aa970bf81450e482a 

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


Testing
---

New test framework for shared resources added.
Tests successful.


Thanks,

Anindya Sinha

Re: Review Request 45962: Updated a persistent volume test framework to include shared volumes.

[Jiang Yan Xu]

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


Ship it!




I can take care of the addressing remaining minor comments.


src/examples/persistent_volume_framework.cpp (line 219)


s/consumer/producer/.



src/examples/persistent_volume_framework.cpp (line 242)


The empty line here doesn't comply with the [style 
guide](https://google.github.io/styleguide/cppguide.html#Loops_and_Switch_Statements).
 Also for long blocks of switch case it looks better with `{}`.



src/examples/persistent_volume_framework.cpp (line 279)


`)~");` should be aligned with `tasks` above but more usually we just put 
one the same line where the string ends.



src/examples/persistent_volume_framework.cpp (line 431)


s/and terminates/(terminated)/ since the last state is already TERMINATING.



src/examples/persistent_volume_framework.cpp (line 438)


s/initialized/initial/



src/examples/persistent_volume_framework.cpp (line 439)


Due to the change to add the INIT state, I think we have to revise this a 
bit:

```
STAGING, // The shard has started but haven't finished launching tasks.
```


- Jiang Yan Xu


On Dec. 2, 2016, 1:43 p.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45962/
> ---
> 
> (Updated Dec. 2, 2016, 1:43 p.m.)
> 
> 
> Review request for mesos, Greg Mann, Jie Yu, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-4431
> https://issues.apache.org/jira/browse/MESOS-4431
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated a persistent volume test framework to include shared volumes.
> 
> 
> Diffs
> -
> 
>   src/examples/persistent_volume_framework.cpp 
> 9d45bb496c4cf378af429b5aa970bf81450e482a 
> 
> Diff: https://reviews.apache.org/r/45962/diff/
> 
> 
> Testing
> ---
> 
> New test framework for shared resources added.
> Tests successful.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 45962: Updated a persistent volume test framework to include shared volumes.

[Anindya Sinha]

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

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


Review request for mesos, Greg Mann, Jie Yu, and Jiang Yan Xu.


Changes
---

rebase.


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


Repository: mesos


Description
---

Updated a persistent volume test framework to include shared volumes.


Diffs (updated)
-

  src/examples/persistent_volume_framework.cpp 
9d45bb496c4cf378af429b5aa970bf81450e482a 

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


Testing
---

New test framework for shared resources added.
Tests successful.


Thanks,

Anindya Sinha

Re: Review Request 45962: Updated a persistent volume test framework to include shared volumes.

[Anindya Sinha]

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

(Updated Dec. 2, 2016, 12:02 a.m.)


Review request for mesos, Greg Mann, Jie Yu, and Jiang Yan Xu.


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


Repository: mesos


Description
---

Updated a persistent volume test framework to include shared volumes.


Diffs (updated)
-

  src/examples/persistent_volume_framework.cpp 
9d45bb496c4cf378af429b5aa970bf81450e482a 

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


Testing
---

New test framework for shared resources added.
Tests successful.


Thanks,

Anindya Sinha

Re: Review Request 45962: Updated a persistent volume test framework to include shared volumes.

[Anindya Sinha]

> On Nov. 18, 2016, 11:56 a.m., Gastón Kleiman wrote:
> > src/examples/persistent_volume_framework.cpp, line 150
> > 
> >
> > Would it make sense to add the `shared-vol` prefix to the shard name in 
> > order to make debugging/troubleshooting easier?

Yes, agreed. Updated.


- Anindya


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


On Nov. 29, 2016, 12:25 a.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45962/
> ---
> 
> (Updated Nov. 29, 2016, 12:25 a.m.)
> 
> 
> Review request for mesos, Greg Mann, Jie Yu, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-4431
> https://issues.apache.org/jira/browse/MESOS-4431
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated a persistent volume test framework to include shared volumes.
> 
> 
> Diffs
> -
> 
>   src/examples/persistent_volume_framework.cpp 
> 9d45bb496c4cf378af429b5aa970bf81450e482a 
> 
> Diff: https://reviews.apache.org/r/45962/diff/
> 
> 
> Testing
> ---
> 
> New test framework for shared resources added.
> Tests successful.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 45962: Updated a persistent volume test framework to include shared volumes.

[Anindya Sinha]

> On Dec. 1, 2016, 9:29 p.m., Greg Mann wrote:
> > src/examples/persistent_volume_framework.cpp, lines 427-430
> > 
> >
> > Could you provide comments here or elsewhere which give a high-level 
> > overview of the state machine's strategy. i.e., first we launch a "writer 
> > task" that touches a file, then we use "reader tasks" to verify volume 
> > functionality.

Added a comment on the flow of the state machine in 
`PersistentVolumeScheduler::resourceOffers()` [where the state machine is 
initiated and managed].


- Anindya


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


On Nov. 29, 2016, 12:25 a.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45962/
> ---
> 
> (Updated Nov. 29, 2016, 12:25 a.m.)
> 
> 
> Review request for mesos, Greg Mann, Jie Yu, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-4431
> https://issues.apache.org/jira/browse/MESOS-4431
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated a persistent volume test framework to include shared volumes.
> 
> 
> Diffs
> -
> 
>   src/examples/persistent_volume_framework.cpp 
> 9d45bb496c4cf378af429b5aa970bf81450e482a 
> 
> Diff: https://reviews.apache.org/r/45962/diff/
> 
> 
> Testing
> ---
> 
> New test framework for shared resources added.
> Tests successful.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 45962: Updated a persistent volume test framework to include shared volumes.

[Anindya Sinha]

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

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


Review request for mesos, Greg Mann, Jie Yu, and Jiang Yan Xu.


Changes
---

addressed review comments.


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


Repository: mesos


Description
---

Updated a persistent volume test framework to include shared volumes.


Diffs (updated)
-

  src/examples/persistent_volume_framework.cpp 
9d45bb496c4cf378af429b5aa970bf81450e482a 

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


Testing
---

New test framework for shared resources added.
Tests successful.


Thanks,

Anindya Sinha

Re: Review Request 45962: Updated a persistent volume test framework to include shared volumes.

[Greg Mann]

> On Dec. 1, 2016, 9:29 p.m., Greg Mann wrote:
> > src/examples/persistent_volume_framework.cpp, lines 502-508
> > 
> >
> > I would suggest that we should run this test framework in the test 
> > suite with both regular and shared persistent volumes. We could:
> > 1) Do a single run of the test framework which uses both shared and 
> > regular volumes
> > 2) Do a couple different runs of the test framework which each use 
> > different types of volumes.
> > 
> > This could be accomplished either here in the default flag values, or 
> > by altering the test run script to pass flags in.
> > 
> > Thoughts?

Sorry! As you pointed out offline, you're already doing this :) Dropping the 
issue.


- Greg


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


On Nov. 29, 2016, 12:25 a.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45962/
> ---
> 
> (Updated Nov. 29, 2016, 12:25 a.m.)
> 
> 
> Review request for mesos, Greg Mann, Jie Yu, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-4431
> https://issues.apache.org/jira/browse/MESOS-4431
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated a persistent volume test framework to include shared volumes.
> 
> 
> Diffs
> -
> 
>   src/examples/persistent_volume_framework.cpp 
> 9d45bb496c4cf378af429b5aa970bf81450e482a 
> 
> Diff: https://reviews.apache.org/r/45962/diff/
> 
> 
> Testing
> ---
> 
> New test framework for shared resources added.
> Tests successful.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 45962: Updated a persistent volume test framework to include shared volumes.

[Greg Mann]

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


Fix it, then Ship it!





src/examples/persistent_volume_framework.cpp (lines 121 - 127)


s/slave/agent



src/examples/persistent_volume_framework.cpp (line 275)


This is unnecessary since we're in the `case` block for `Shard::STAGING` 
already.



src/examples/persistent_volume_framework.cpp (lines 412 - 415)


Could you provide comments here or elsewhere which give a high-level 
overview of the state machine's strategy. i.e., first we launch a "writer task" 
that touches a file, then we use "reader tasks" to verify volume functionality.



src/examples/persistent_volume_framework.cpp (lines 483 - 489)


I would suggest that we should run this test framework in the test suite 
with both regular and shared persistent volumes. We could:
1) Do a single run of the test framework which uses both shared and regular 
volumes
2) Do a couple different runs of the test framework which each use 
different types of volumes.

This could be accomplished either here in the default flag values, or by 
altering the test run script to pass flags in.

Thoughts?


- Greg Mann


On Nov. 29, 2016, 12:25 a.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45962/
> ---
> 
> (Updated Nov. 29, 2016, 12:25 a.m.)
> 
> 
> Review request for mesos, Greg Mann, Jie Yu, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-4431
> https://issues.apache.org/jira/browse/MESOS-4431
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated a persistent volume test framework to include shared volumes.
> 
> 
> Diffs
> -
> 
>   src/examples/persistent_volume_framework.cpp 
> 9d45bb496c4cf378af429b5aa970bf81450e482a 
> 
> Diff: https://reviews.apache.org/r/45962/diff/
> 
> 
> Testing
> ---
> 
> New test framework for shared resources added.
> Tests successful.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 45962: Updated a persistent volume test framework to include shared volumes.

[Jiang Yan Xu]

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


Fix it, then Ship it!




Looks good! :)


src/examples/persistent_volume_framework.cpp (line 86)


An empty line here before the return.



src/examples/persistent_volume_framework.cpp (line 122)


s/In case/In the case/



src/examples/persistent_volume_framework.cpp (line 124)


s/in case/in the case/



src/examples/persistent_volume_framework.cpp (line 150)


As Gaston suggested, embeding sharedness in the name probably makes it more 
readable in the logs. Maybe `"shared-shard-"`?

If we do that, no need to do `i + numShards`? Just do `stringify(i)`?



src/examples/persistent_volume_framework.cpp (lines 196 - 198)


Use `Shard::INIT` as discussed.

Then we can probably remove the comment. Consider the state machine already 
initialized (to INIT).



src/examples/persistent_volume_framework.cpp (lines 217 - 218)


Add a TODO for the custom write command flag here to pair with the TODO 
below?



src/examples/persistent_volume_framework.cpp (line 254)


Move this empty line to above line 253?



src/examples/persistent_volume_framework.cpp (lines 264 - 265)


Can we briefly explain the reason: the writer task, although launched by 
the scheduler first, may have yet to write to the disk.



src/examples/persistent_volume_framework.cpp (line 267)


Nit: "specifying a custom" reads better than "specifying a specific"?



src/examples/persistent_volume_framework.cpp (lines 269 - 272)


Can we use 

```
R"~(
)~"
```

string so we can write the script with with line breaks and indentation as 
we would usually do?


- Jiang Yan Xu


On Nov. 28, 2016, 4:25 p.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45962/
> ---
> 
> (Updated Nov. 28, 2016, 4:25 p.m.)
> 
> 
> Review request for mesos, Greg Mann, Jie Yu, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-4431
> https://issues.apache.org/jira/browse/MESOS-4431
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated a persistent volume test framework to include shared volumes.
> 
> 
> Diffs
> -
> 
>   src/examples/persistent_volume_framework.cpp 
> 9d45bb496c4cf378af429b5aa970bf81450e482a 
> 
> Diff: https://reviews.apache.org/r/45962/diff/
> 
> 
> Testing
> ---
> 
> New test framework for shared resources added.
> Tests successful.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 45962: Updated a persistent volume test framework to include shared volumes.

[Jiang Yan Xu]

> On Nov. 17, 2016, 9:20 a.m., Jiang Yan Xu wrote:
> > src/examples/persistent_volume_framework.cpp, line 201
> > 
> >
> > Consolidate this with `case Shard::STAGING`? 
> > 
> > We can just do:
> > 
> > ```
> > if (shard.launched == 0) {
> >   // Create volume, add to `operations`.
> >   // Update info in Shard.
> >   // Add producer task to operations.
> > } else {
> >   // Modify volume to RO.
> >   // Add consumer task to operations.
> > }
> > ```
> 
> Anindya Sinha wrote:
> To do this, we need to init `Shard` with state of `Shard::STAGING`. I do 
> not think we should init state to `Shard::STAGING` (syntactically incorrect). 
> So, either we have a `Shard::INIT` (like we had before) or use the fact that 
> `Shard.state` is `None()`.
> So, if we do not want to add back `Shard::INIT`, we would need to handle 
> the case of the 1st task outside the `switch`.

Summarizing offline discussion: my intention was to eliminate states that are 
not "actionable" (the code sets them but doesn't act differently based on 
them). Anindya's point was that as state machines we better have a distinct 
start and end state. We can add Shard::INIT back.


- Jiang Yan


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


On Nov. 28, 2016, 4:25 p.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45962/
> ---
> 
> (Updated Nov. 28, 2016, 4:25 p.m.)
> 
> 
> Review request for mesos, Greg Mann, Jie Yu, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-4431
> https://issues.apache.org/jira/browse/MESOS-4431
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated a persistent volume test framework to include shared volumes.
> 
> 
> Diffs
> -
> 
>   src/examples/persistent_volume_framework.cpp 
> 9d45bb496c4cf378af429b5aa970bf81450e482a 
> 
> Diff: https://reviews.apache.org/r/45962/diff/
> 
> 
> Testing
> ---
> 
> New test framework for shared resources added.
> Tests successful.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 45962: Updated a persistent volume test framework to include shared volumes.

[Anindya Sinha]

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

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


Review request for mesos, Greg Mann, Jie Yu, and Jiang Yan Xu.


Changes
---

Rebased.


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


Repository: mesos


Description
---

Updated a persistent volume test framework to include shared volumes.


Diffs (updated)
-

  src/examples/persistent_volume_framework.cpp 
9d45bb496c4cf378af429b5aa970bf81450e482a 

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


Testing
---

New test framework for shared resources added.
Tests successful.


Thanks,

Anindya Sinha

Re: Review Request 45962: Updated a persistent volume test framework to include shared volumes.

[Anindya Sinha]

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

(Updated Nov. 21, 2016, 8:11 a.m.)


Review request for mesos, Greg Mann, Jie Yu, and Jiang Yan Xu.


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


Repository: mesos


Description
---

Updated a persistent volume test framework to include shared volumes.


Diffs (updated)
-

  src/examples/persistent_volume_framework.cpp 
9d45bb496c4cf378af429b5aa970bf81450e482a 

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


Testing
---

New test framework for shared resources added.
Tests successful.


Thanks,

Anindya Sinha

Re: Review Request 45962: Updated a persistent volume test framework to include shared volumes.

[Gastón Kleiman]

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




src/examples/persistent_volume_framework.cpp (line 150)


Would it make sense to add the `shared-vol` prefix to the shard name in 
order to make debugging/troubleshooting easier?


- Gastón Kleiman


On Nov. 18, 2016, 5:13 a.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45962/
> ---
> 
> (Updated Nov. 18, 2016, 5:13 a.m.)
> 
> 
> Review request for mesos, Greg Mann, Jie Yu, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-4431
> https://issues.apache.org/jira/browse/MESOS-4431
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated a persistent volume test framework to include shared volumes.
> 
> 
> Diffs
> -
> 
>   src/examples/persistent_volume_framework.cpp 
> 9d45bb496c4cf378af429b5aa970bf81450e482a 
> 
> Diff: https://reviews.apache.org/r/45962/diff/
> 
> 
> Testing
> ---
> 
> New test framework for shared resources added.
> Tests successful.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 45962: Updated a persistent volume test framework to include shared volumes.

[Gastón Kleiman]

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




src/examples/persistent_volume_framework.cpp (line 265)


s/upto/up to/



src/examples/persistent_volume_framework.cpp (line 280)


s/create/created/



src/examples/persistent_volume_framework.cpp (line 289)


Missing space before the opening curly brace.


- Gastón Kleiman


On Nov. 18, 2016, 5:13 a.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45962/
> ---
> 
> (Updated Nov. 18, 2016, 5:13 a.m.)
> 
> 
> Review request for mesos, Greg Mann, Jie Yu, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-4431
> https://issues.apache.org/jira/browse/MESOS-4431
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated a persistent volume test framework to include shared volumes.
> 
> 
> Diffs
> -
> 
>   src/examples/persistent_volume_framework.cpp 
> 9d45bb496c4cf378af429b5aa970bf81450e482a 
> 
> Diff: https://reviews.apache.org/r/45962/diff/
> 
> 
> Testing
> ---
> 
> New test framework for shared resources added.
> Tests successful.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 45962: Updated a persistent volume test framework to include shared volumes.

[Anindya Sinha]

> On Nov. 17, 2016, 5:20 p.m., Jiang Yan Xu wrote:
> > src/examples/persistent_volume_framework.cpp, line 201
> > 
> >
> > Consolidate this with `case Shard::STAGING`? 
> > 
> > We can just do:
> > 
> > ```
> > if (shard.launched == 0) {
> >   // Create volume, add to `operations`.
> >   // Update info in Shard.
> >   // Add producer task to operations.
> > } else {
> >   // Modify volume to RO.
> >   // Add consumer task to operations.
> > }
> > ```

To do this, we need to init `Shard` with state of `Shard::STAGING`. I do not 
think we should init state to `Shard::STAGING` (syntactically incorrect). So, 
either we have a `Shard::INIT` (like we had before) or use the fact that 
`Shard.state` is `None()`.
So, if we do not want to add back `Shard::INIT`, we would need to handle the 
case of the 1st task outside the `switch`.


- Anindya


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


On Nov. 18, 2016, 5:13 a.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45962/
> ---
> 
> (Updated Nov. 18, 2016, 5:13 a.m.)
> 
> 
> Review request for mesos, Greg Mann, Jie Yu, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-4431
> https://issues.apache.org/jira/browse/MESOS-4431
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated a persistent volume test framework to include shared volumes.
> 
> 
> Diffs
> -
> 
>   src/examples/persistent_volume_framework.cpp 
> 9d45bb496c4cf378af429b5aa970bf81450e482a 
> 
> Diff: https://reviews.apache.org/r/45962/diff/
> 
> 
> Testing
> ---
> 
> New test framework for shared resources added.
> Tests successful.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 45962: Updated a persistent volume test framework to include shared volumes.

[Anindya Sinha]

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

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


Review request for mesos, Greg Mann, Jie Yu, and Jiang Yan Xu.


Changes
---

addressed review comments.


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


Repository: mesos


Description
---

Updated a persistent volume test framework to include shared volumes.


Diffs (updated)
-

  src/examples/persistent_volume_framework.cpp 
9d45bb496c4cf378af429b5aa970bf81450e482a 

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


Testing
---

New test framework for shared resources added.
Tests successful.


Thanks,

Anindya Sinha

Re: Review Request 45962: Updated a persistent volume test framework to include shared volumes.

[Jiang Yan Xu]

> On Nov. 6, 2016, 9:25 p.m., Jiang Yan Xu wrote:
> > src/examples/persistent_volume_framework.cpp, lines 470-476
> > 
> >
> > I think it's sufficient to have the following states. (We should use a 
> > minimun number of state, we can always add more when more features are 
> > added to persistent volumes which demand more state but starting with a 
> > large number of states makes it difficult to evolve the test).
> > 
> > ```
> >   STAGING = 0, // The shard is awaiting offers to launch more tasks.
> >   RUNNING, // The shard is fully running (all its tasks are 
> > launched).
> >   TERMINATING, // The shard is terminating and needs to clean up 
> > its persistent volume.
> >   DONE // The shard is terminated.
> > ```
> > 
> > This translates to:
> > 
> > ```
> >   STAGING = 0, // In resourceOffers: launch tasks
> >   RUNNING, // In resourceOffers: noop; In statusUpdate: check 
> > shard TERMINATING condition.
> >   TERMINATING, // In resourceOffers: DESTROY
> >   DONE // Test terminal condition.
> > ```
> 
> Anindya Sinha wrote:
> To accomplish this state, I changed shard's state to `Option 
> state` so we start the state machine if `shared.state == None()`. Also, if 
> for some reason `DESTROY` fails continuously, the test would not exit. We can 
> fail the test if it fails on n attempts of `DESTROY` by keeping track of 
> number of `DESTROY` attempts but maybe not required. Comments?
> 
> Jiang Yan Xu wrote:
> Keeping track of the number of destroys may be too much IMO :) We don't 
> terminate when the agent fails to receive the tasks either (we don't 
> reconcile) but I think these scenarios are too rear for a simple example 
> framework.

s/rear/rare/ :)


- Jiang Yan


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


On Nov. 7, 2016, 9:50 p.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45962/
> ---
> 
> (Updated Nov. 7, 2016, 9:50 p.m.)
> 
> 
> Review request for mesos, Greg Mann, Jie Yu, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-4431
> https://issues.apache.org/jira/browse/MESOS-4431
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated a persistent volume test framework to include shared volumes.
> 
> 
> Diffs
> -
> 
>   src/examples/persistent_volume_framework.cpp 
> 9d45bb496c4cf378af429b5aa970bf81450e482a 
> 
> Diff: https://reviews.apache.org/r/45962/diff/
> 
> 
> Testing
> ---
> 
> New test framework for shared resources added.
> Tests successful.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 45962: Updated a persistent volume test framework to include shared volumes.

[Jiang Yan Xu]

> On Nov. 6, 2016, 9:25 p.m., Jiang Yan Xu wrote:
> > src/examples/persistent_volume_framework.cpp, lines 470-476
> > 
> >
> > I think it's sufficient to have the following states. (We should use a 
> > minimun number of state, we can always add more when more features are 
> > added to persistent volumes which demand more state but starting with a 
> > large number of states makes it difficult to evolve the test).
> > 
> > ```
> >   STAGING = 0, // The shard is awaiting offers to launch more tasks.
> >   RUNNING, // The shard is fully running (all its tasks are 
> > launched).
> >   TERMINATING, // The shard is terminating and needs to clean up 
> > its persistent volume.
> >   DONE // The shard is terminated.
> > ```
> > 
> > This translates to:
> > 
> > ```
> >   STAGING = 0, // In resourceOffers: launch tasks
> >   RUNNING, // In resourceOffers: noop; In statusUpdate: check 
> > shard TERMINATING condition.
> >   TERMINATING, // In resourceOffers: DESTROY
> >   DONE // Test terminal condition.
> > ```
> 
> Anindya Sinha wrote:
> To accomplish this state, I changed shard's state to `Option 
> state` so we start the state machine if `shared.state == None()`. Also, if 
> for some reason `DESTROY` fails continuously, the test would not exit. We can 
> fail the test if it fails on n attempts of `DESTROY` by keeping track of 
> number of `DESTROY` attempts but maybe not required. Comments?

Keeping track of the number of destroys may be too much IMO :) We don't 
terminate when the agent fails to receive the tasks either (we don't reconcile) 
but I think these scenarios are too rear for a simple example framework.


- Jiang Yan


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


On Nov. 7, 2016, 9:50 p.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45962/
> ---
> 
> (Updated Nov. 7, 2016, 9:50 p.m.)
> 
> 
> Review request for mesos, Greg Mann, Jie Yu, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-4431
> https://issues.apache.org/jira/browse/MESOS-4431
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated a persistent volume test framework to include shared volumes.
> 
> 
> Diffs
> -
> 
>   src/examples/persistent_volume_framework.cpp 
> 9d45bb496c4cf378af429b5aa970bf81450e482a 
> 
> Diff: https://reviews.apache.org/r/45962/diff/
> 
> 
> Testing
> ---
> 
> New test framework for shared resources added.
> Tests successful.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 45962: Updated a persistent volume test framework to include shared volumes.

[Jiang Yan Xu]

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




src/examples/persistent_volume_framework.cpp (lines 33 - 37)


Are these used?



src/examples/persistent_volume_framework.cpp (line 128)


`immediately` is not accurate? (it still needs to wait until the next offer 
cycle). 

I guess it sufficies to just say that "in the case of shared persistent 
volumes, tasks can access the shared volume simutaneously".



src/examples/persistent_volume_framework.cpp (line 201)


Consolidate this with `case Shard::STAGING`? 

We can just do:

```
if (shard.launched == 0) {
  // Create volume, add to `operations`.
  // Update info in Shard.
  // Add producer task to operations.
} else {
  // Modify volume to RO.
  // Add consumer task to operations.
}
```



src/examples/persistent_volume_framework.cpp (lines 205 - 208)


Remove this?



src/examples/persistent_volume_framework.cpp (lines 235 - 236)


Kill this since it's agnostic about whether the volume is shared?



src/examples/persistent_volume_framework.cpp (lines 259 - 267)


Is this necessary? We use unique persistence IDs.



src/examples/persistent_volume_framework.cpp (lines 269 - 280)


The need to look up the volume Resource here and below in TERMINATING 
justifies saving it in `Volume`?

i.e.,

```
struct Volume
{
  Option resource;
  bool isShared;
}
```

(Doesn't look like we even need to record the persistent ID or SlaveID 
separately?)

Then here:

```
Resource volume = shard.volume.resources.get();

// Strip out the volume; modify mode; add it back.
Resources taskResources = shard.resources - volume;
volume.mutable_disk()->mutable_volume()->set_mode(Volume::RO);
taskResources += volume;
```

It simplifies the DESTROY logic as well.



src/examples/persistent_volume_framework.cpp (lines 292 - 300)


Can we consolidate the two into one and use a 15 second timeout (a more 
standard timeout in Mesos tests)?

The consumer command in the case of a shared persistent volume should work 
in the regular persistent volume case. When we allow it to be specified via a 
flag, there should be just a single flag. (A TODO for it is fine, although it 
should be pretty trivial to add it)



src/examples/persistent_volume_framework.cpp (lines 313 - 315)


Seems like we can rely on the state `Shard::TERMINATING` to guarantee 
`shard.terminated == shard.tasks` if we define it as "all tasks have 
terminated" and transition the state accordingly.



src/examples/persistent_volume_framework.cpp (lines 325 - 327)


This `operations` is for all shards, `operations.size() == 0` is not 
intuitive and is going to take multiple cycles to settle.

Can we just directly translate the condition for DONE into code: "the 
shard's persistent volume no longer exists in the offer when the state is 
TERMINATING".

If we store the volume resource in `Volume` like I suggested above, we can 
just do `if (!offered.contains(volume)) { /* Done. */ }`.



src/examples/persistent_volume_framework.cpp (lines 372 - 374)


Can we just use `hashset` for taskIds given we have to do this?



src/examples/persistent_volume_framework.cpp (lines 377 - 383)


No need to differentiate since we have defined `Shard::RUNNING` to mean 
"The shard is running (i.e., **all** tasks have launched)". This can apply to 
nonshared persistent volumes too.

So we can 

```
if (shard.launched == shard.tasks &&
shard.state == Shard::STAGING) {
  shard.state = Shard::RUNNING;
}
```



src/examples/persistent_volume_framework.cpp (line 378)


Can `shard.launched > shard.tasks`? If impossible you can `CHECK`.



src/examples/persistent_volume_framework.cpp (lines 387 - 396)


For nonshared persistent volumes no need for transition

STAGING -> RUNNING -> STAGING -> RUNNING ... -> TERMINATING

The following can work in both cases?

```
if 

Re: Review Request 45962: Updated a persistent volume test framework to include shared volumes.

[Anindya Sinha]

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

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


Review request for mesos, Greg Mann, Jie Yu, and Jiang Yan Xu.


Changes
---

Addressed review comments.


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


Repository: mesos


Description
---

Updated a persistent volume test framework to include shared volumes.


Diffs (updated)
-

  src/examples/persistent_volume_framework.cpp 
9d45bb496c4cf378af429b5aa970bf81450e482a 

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


Testing
---

New test framework for shared resources added.
Tests successful.


Thanks,

Anindya Sinha

Re: Review Request 45962: Updated a persistent volume test framework to include shared volumes.

[Anindya Sinha]

> On Nov. 7, 2016, 5:25 a.m., Jiang Yan Xu wrote:
> > src/examples/persistent_volume_framework.cpp, line 129
> > 
> >
> > Keep the name?
> > 
> > Shared persistent volume is a kind of persistent volume.

Yes, agreed. This happened since to start off, we had a separate test framework 
for shared volumes, and later decided to merge the test frameworks for shared 
and regular persistent volumes into a single test.


> On Nov. 7, 2016, 5:25 a.m., Jiang Yan Xu wrote:
> > src/examples/persistent_volume_framework.cpp, lines 188-189
> > 
> >
> > Why is "shared-only" a possible value? A shard has only one persistent 
> > volume so it's either "shared" or not. How about just a `bool isShared`. 
> > (As suggested below, we can put it in the volume).

The original thinking for `flags.mode` was not scoped to a shard but to the 
framework, i.e. if the framework launches shards with tasks where every shard 
used a shared volume (`shared-only` case) or alternates between shared and 
regular volumes (`mixed` case). However, I think I liked separating that 
intention with `num_shards` (for regular volumes) and `num_shared_shards` (for 
shared volumes). So, removed `flags.mode` and added `flags.num_shared_shards`.


> On Nov. 7, 2016, 5:25 a.m., Jiang Yan Xu wrote:
> > src/examples/persistent_volume_framework.cpp, lines 215-222
> > 
> >
> > No need to act differently based on the shard type.
> > 
> > We can just have 
> > 
> > - The first task (`launched == 0`) writes to the file: `echo hello > 
> > volume/persisted`
> > - Later tasks read from the file: `cat volume/persisted`
> > 
> > For regular persistent volumes the two tasks will be sequential, for 
> > shared ones they'll be simulatenous. The fact that 1st task could finish 
> > before the 2nd is launched is not important: the example framework mainly 
> > serves the purpose of demostrating the usage of the feature and we don't 
> > want the tasks to block the CI for too long.
> > 
> > As a potentially followup we can take a read/consumer command and a 
> > write/producer command from flags (with default values). So CI would use 
> > the default values to complete quickly while a human could try out 
> > different write and reads commands which could represent more 
> > interesting/edge cases.

I think the issue is when the writer task has not done an actual write yet, but 
the reader task has done a read already (this is possible since these are 2 
separate tasks). This would not happen in shards using regular volumes (since 
we guarantee the order of task launches and hence order of read/write), but can 
happen in rare case in shared volumes. So, this is what I did to avoid that 
race:

Writer task:
`echo hello > volume/persisted` for both regular and shared volumes.

Reader task(s):
Regular volumes: `cat volume/persisted`
Shared volumes: We try for 5 iterations to read `volume/persisted` and we exit 
when we read successfully (exits with success) or we fail to read in 5 
iterations (exits with a failure).
```COUNTER=0; while [ $COUNTER -lt 5 ]; 
   do cat volume/persisted;
   if [ $? -eq 0 ]; then
 exit 0;
   fi;
   COUNTER=$[COUNTER+1]; sleep 1; done; exit 1");```


> On Nov. 7, 2016, 5:25 a.m., Jiang Yan Xu wrote:
> > src/examples/persistent_volume_framework.cpp, lines 208-209
> > 
> >
> > Any need for `string task_id`? If so it needs to be named `taskId` but 
> > I don't see the need? Once it's assigned to the task we can get it out by 
> > `task.task_id()`.

I was doing a `echo` on the `task id` so I used a temporary variable, but I 
agree we can certainly avoid it.


> On Nov. 7, 2016, 5:25 a.m., Jiang Yan Xu wrote:
> > src/examples/persistent_volume_framework.cpp, lines 470-476
> > 
> >
> > I think it's sufficient to have the following states. (We should use a 
> > minimun number of state, we can always add more when more features are 
> > added to persistent volumes which demand more state but starting with a 
> > large number of states makes it difficult to evolve the test).
> > 
> > ```
> >   STAGING = 0, // The shard is awaiting offers to launch more tasks.
> >   RUNNING, // The shard is fully running (all its tasks are 
> > launched).
> >   TERMINATING, // The shard is terminating and needs to clean up 
> > its persistent volume.
> >   DONE // The shard is terminated.
> > ```
> > 
> > This translates to:
> > 
> > ```
> >   STAGING = 0, // In resourceOffers: launch tasks
> >   RUNNING, // In resourceOffers: noop; In 

Re: Review Request 45962: Updated a persistent volume test framework to include shared volumes.

[Jiang Yan Xu]

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




src/examples/persistent_volume_framework.cpp 


Keep the TODO?



src/examples/persistent_volume_framework.cpp (line 123)


s/shared/(possibly shared)/



src/examples/persistent_volume_framework.cpp (line 127)


Keep the name?

Shared persistent volume is a kind of persistent volume.



src/examples/persistent_volume_framework.cpp (lines 137 - 143)


Here we can just create regular shards and shared shards based on input and 
one after another. No interleaving necessary. See comments on the flags below.



src/examples/persistent_volume_framework.cpp (lines 186 - 187)


Why is "shared-only" a possible value? A shard has only one persistent 
volume so it's either "shared" or not. How about just a `bool isShared`. (As 
suggested below, we can put it in the volume).



src/examples/persistent_volume_framework.cpp (lines 206 - 207)


Any need for `string task_id`? If so it needs to be named `taskId` but I 
don't see the need? Once it's assigned to the task we can get it out by 
`task.task_id()`.



src/examples/persistent_volume_framework.cpp (lines 213 - 220)


No need to act differently based on the shard type.

We can just have 

- The first task (`launched == 0`) writes to the file: `echo hello > 
volume/persisted`
- Later tasks read from the file: `cat volume/persisted`

For regular persistent volumes the two tasks will be sequential, for shared 
ones they'll be simulatenous. The fact that 1st task could finish before the 
2nd is launched is not important: the example framework mainly serves the 
purpose of demostrating the usage of the feature and we don't want the tasks to 
block the CI for too long.

As a potentially followup we can take a read/consumer command and a 
write/producer command from flags (with default values). So CI would use the 
default values to complete quickly while a human could try out different write 
and reads commands which could represent more interesting/edge cases.



src/examples/persistent_volume_framework.cpp (lines 455 - 461)


I think it's sufficient to have the following states. (We should use a 
minimun number of state, we can always add more when more features are added to 
persistent volumes which demand more state but starting with a large number of 
states makes it difficult to evolve the test).

```
  STAGING = 0, // The shard is awaiting offers to launch more tasks.
  RUNNING, // The shard is fully running (all its tasks are 
launched).
  TERMINATING, // The shard is terminating and needs to clean up its 
persistent volume.
  DONE // The shard is terminated.
```

This translates to:

```
  STAGING = 0, // In resourceOffers: launch tasks
  RUNNING, // In resourceOffers: noop; In statusUpdate: check shard 
TERMINATING condition.
  TERMINATING, // In resourceOffers: DESTROY
  DONE // Test terminal condition.
```



src/examples/persistent_volume_framework.cpp (line 458)






src/examples/persistent_volume_framework.cpp (line 459)


To represent a state, use a `-ing` word?

See the overall comment on the enum.



src/examples/persistent_volume_framework.cpp (lines 460 - 461)


Why both `WAIT_DONE` and `DONE`?

Doesn't look like we need to treat shared pv and regular pv differently.

See the overall comment on the enum.



src/examples/persistent_volume_framework.cpp (lines 465 - 475)


I understand this is "just a test" but it would be cleaner the following 
way.

```
struct Volume
{
  explicit Volume(bool _isShared)
: isShared(_isShared) {}
  
  Option id;
  Option slave;
  bool isShared;
}
```

Here the volume is really the "intention" for the persistent volume so 
`isShared` is known when the shard is created and the optional values are 
filled when known.



src/examples/persistent_volume_framework.cpp (line 495)


s/taskId/taskIds/?

```
// The IDs of the tasks running on 

Re: Review Request 45962: Updated a persistent volume test framework to include shared volumes.

[Anindya Sinha]

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

(Updated Oct. 21, 2016, 6:52 p.m.)


Review request for mesos and Jiang Yan Xu.


Changes
---

Consolidated shared and regular persistent example frameworks into a single 
framework.


Summary (updated)
-

Updated a persistent volume test framework to include shared volumes.


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


Repository: mesos


Description (updated)
---

Updated a persistent volume test framework to include shared volumes.


Diffs (updated)
-

  src/examples/persistent_volume_framework.cpp 
9d45bb496c4cf378af429b5aa970bf81450e482a 

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


Testing
---

New test framework for shared resources added.
Tests successful.


Thanks,

Anindya Sinha