----------------------------------------------------------- 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) <https://reviews.apache.org/r/45962/#comment226370> Are these used? src/examples/persistent_volume_framework.cpp (line 128) <https://reviews.apache.org/r/45962/#comment226217> `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) <https://reviews.apache.org/r/45962/#comment226277> 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) <https://reviews.apache.org/r/45962/#comment226278> Remove this? src/examples/persistent_volume_framework.cpp (lines 235 - 236) <https://reviews.apache.org/r/45962/#comment226279> Kill this since it's agnostic about whether the volume is shared? src/examples/persistent_volume_framework.cpp (lines 259 - 267) <https://reviews.apache.org/r/45962/#comment226290> Is this necessary? We use unique persistence IDs. src/examples/persistent_volume_framework.cpp (lines 269 - 280) <https://reviews.apache.org/r/45962/#comment226372> The need to look up the volume Resource here and below in TERMINATING justifies saving it in `Volume`? i.e., ``` struct Volume { Option<Resource> 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) <https://reviews.apache.org/r/45962/#comment226300> 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) <https://reviews.apache.org/r/45962/#comment226374> 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) <https://reviews.apache.org/r/45962/#comment226328> 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) <https://reviews.apache.org/r/45962/#comment226329> Can we just use `hashset` for taskIds given we have to do this? src/examples/persistent_volume_framework.cpp (lines 377 - 383) <https://reviews.apache.org/r/45962/#comment226330> 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) <https://reviews.apache.org/r/45962/#comment226331> Can `shard.launched > shard.tasks`? If impossible you can `CHECK`. src/examples/persistent_volume_framework.cpp (lines 387 - 396) <https://reviews.apache.org/r/45962/#comment226334> For nonshared persistent volumes no need for transition STAGING -> RUNNING -> STAGING -> RUNNING ... -> TERMINATING The following can work in both cases? ``` if (shard.terminated == shard.tasks && shard.state = Shard::RUNNING) { shard.state = Shard::TERMINATING; } ``` src/examples/persistent_volume_framework.cpp (line 455) <https://reviews.apache.org/r/45962/#comment226336> s/and/i.e.,/ since the latter is explaining the former. Or maybe that's too verbose, just "All tasks have launched" would do? src/examples/persistent_volume_framework.cpp (line 456) <https://reviews.apache.org/r/45962/#comment226337> How about: ``` // All tasks are finished and needs cleanup. ``` I think it's worth clarify what the condition is. src/examples/persistent_volume_framework.cpp (line 457) <https://reviews.apache.org/r/45962/#comment226338> How about ``` // The shard has finished cleaning up. ``` src/examples/persistent_volume_framework.cpp (line 485) <https://reviews.apache.org/r/45962/#comment226339> Nit: you could just do ``` volume(isShared), ``` src/examples/persistent_volume_framework.cpp (lines 499 - 500) <https://reviews.apache.org/r/45962/#comment226366> Not used? - Jiang Yan Xu 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 > >
