----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/45962/#review154380 -----------------------------------------------------------
src/examples/persistent_volume_framework.cpp <https://reviews.apache.org/r/45962/#comment223905> Keep the TODO? src/examples/persistent_volume_framework.cpp (line 123) <https://reviews.apache.org/r/45962/#comment223941> s/shared/(possibly shared)/ src/examples/persistent_volume_framework.cpp (line 127) <https://reviews.apache.org/r/45962/#comment223906> Keep the name? Shared persistent volume is a kind of persistent volume. src/examples/persistent_volume_framework.cpp (lines 137 - 143) <https://reviews.apache.org/r/45962/#comment223943> 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) <https://reviews.apache.org/r/45962/#comment223944> 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) <https://reviews.apache.org/r/45962/#comment223960> 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) <https://reviews.apache.org/r/45962/#comment223949> 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) <https://reviews.apache.org/r/45962/#comment223974> 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) <https://reviews.apache.org/r/45962/#comment223966> src/examples/persistent_volume_framework.cpp (line 459) <https://reviews.apache.org/r/45962/#comment223964> To represent a state, use a `-ing` word? See the overall comment on the enum. src/examples/persistent_volume_framework.cpp (lines 460 - 461) <https://reviews.apache.org/r/45962/#comment223963> 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) <https://reviews.apache.org/r/45962/#comment224960> I understand this is "just a test" but it would be cleaner the following way. ``` struct Volume { explicit Volume(bool _isShared) : isShared(_isShared) {} Option<string> id; Option<string> 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) <https://reviews.apache.org/r/45962/#comment223956> s/taskId/taskIds/? ``` // The IDs of the tasks running on this shard? ``` src/examples/persistent_volume_framework.cpp (line 499) <https://reviews.apache.org/r/45962/#comment223979> src/examples/persistent_volume_framework.cpp (lines 533 - 546) <https://reviews.apache.org/r/45962/#comment224959> To avoid any "magic" in determining the type/order/mode this test uses, why don't we just use ``` --num_shards=3 // Number of regular (non-shared) shards the framework will run. --num_shared_shards=3 // Number of shared shards the framework will run. Shared shards are shards which use shared persistent volumes. ``` We can use some default values (3 & 3 as shown above) and the user can customize however they want. src/examples/persistent_volume_framework.cpp (line 581) <https://reviews.apache.org/r/45962/#comment224957> Why remove comments? src/examples/persistent_volume_framework.cpp (line 588) <https://reviews.apache.org/r/45962/#comment224958> Revert this. - Jiang Yan Xu On Oct. 21, 2016, 11:52 a.m., Anindya Sinha wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/45962/ > ----------------------------------------------------------- > > (Updated Oct. 21, 2016, 11:52 a.m.) > > > Review request for mesos 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 > >
