----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/52288/#review151635 -----------------------------------------------------------
src/tests/persistent_volume_tests.cpp (line 1005) <https://reviews.apache.org/r/52288/#comment220073> Add the condition "when the shared persistent volume is destroyed" to summary? e.g., ``` // This test verifies that pending offers with shared persistent volumes are rescinded when the volumes are destroyed. ``` src/tests/persistent_volume_tests.cpp (line 1021) <https://reviews.apache.org/r/52288/#comment220136> This is the beginning of the "body" of the test. This test is long and relatively complicated. We should summarize the steps either here as a numbered list or by separating the test body by "headings" (with a step number to distinguish from the lower-level comments). The latter looks better to me in this case. Somthing like: ``` 1. Framework1 creates the volume and launches a task that uses a portion of the offer. 2. Framework2 connects and receives the offer with the remaining resources and the shared volume. 3. Framework1 kills the task and receives another offer with the shared volume. (Now both frameworks have the volume). 4. Framework2 destroys the volume and the offer for framework1 is recinded. ``` (This slightly differs from your version but it reduces the number of steps I think) src/tests/persistent_volume_tests.cpp (lines 1049 - 1050) <https://reviews.apache.org/r/52288/#comment220137> The following more direct? ``` We use a filter of 0 secs so the resources will be available in the next allocation cycle. ``` src/tests/persistent_volume_tests.cpp (lines 1062 - 1066) <https://reviews.apache.org/r/52288/#comment220110> It's important that the task uses only a portion of cpus/mem so the rest can be offered to framework2. Perhaps emphasize this in the comment as well? The benefit of not using the the shared volume in the task is actually not obvious to me: we kill the task before destroying the volume anyways. If it's for simplicity that we don't use the volume, we should comment on the reason being so. src/tests/persistent_volume_tests.cpp (lines 1068 - 1071) <https://reviews.apache.org/r/52288/#comment220124> Don't reuse the variable `offers` (even though it's reset to pending, this detail is very implicit). src/tests/persistent_volume_tests.cpp (lines 1073 - 1078) <https://reviews.apache.org/r/52288/#comment220111> Are these necessary? We just need a framework2 to be connected to the master and when it receives an offer we are golden right? The task is just a mechanism we use to split offers so we don't care how it's doing. src/tests/persistent_volume_tests.cpp (line 1084) <https://reviews.apache.org/r/52288/#comment220133> Move `filters` to the left by one space. src/tests/persistent_volume_tests.cpp (lines 1093 - 1100) <https://reviews.apache.org/r/52288/#comment220138> Connect framework2 before advancing the clock so framework2 can get the offer immediately? (Even before the clock advancement). This reduces the overall number of allocation cycles we have to advance (and the length of the code) right? src/tests/persistent_volume_tests.cpp (line 1094) <https://reviews.apache.org/r/52288/#comment220119> No need to resume the clock here and `Clock::advance()` below doesn't work without pausing again. src/tests/persistent_volume_tests.cpp (lines 1102 - 1103) <https://reviews.apache.org/r/52288/#comment220120> Since `task` doesn't use all resources, we should expect `famework2` to receive the offer immediately right? src/tests/persistent_volume_tests.cpp (lines 1130 - 1131) <https://reviews.apache.org/r/52288/#comment220132> Why wait on status? It should be sufficient that we just wait on offers as they are the result of the kill. src/tests/persistent_volume_tests.cpp (line 1149) <https://reviews.apache.org/r/52288/#comment220131> s/has/have/ src/tests/persistent_volume_tests.cpp (lines 1152 - 1153) <https://reviews.apache.org/r/52288/#comment220128> Instead of `Times(1)`, we can just ``` Future<Nothing> rescinded; EXPECT_CALL(sched1, offerRescinded(&driver1, _)) .WillOnce(FutureSatisfy(&rescinded)); ``` And wait on it: ``` AWAIT_READY(rescinded); ``` src/tests/persistent_volume_tests.cpp (lines 1155 - 1157) <https://reviews.apache.org/r/52288/#comment220129> No need for this. See below. src/tests/persistent_volume_tests.cpp (lines 1164 - 1174) <https://reviews.apache.org/r/52288/#comment220123> This test is for offer rescind so we can just stop at the expectation for that right? - Jiang Yan Xu On Sept. 26, 2016, 4:58 p.m., Anindya Sinha wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/52288/ > ----------------------------------------------------------- > > (Updated Sept. 26, 2016, 4:58 p.m.) > > > Review request for mesos and Jiang Yan Xu. > > > Bugs: MESOS-6257 > https://issues.apache.org/jira/browse/MESOS-6257 > > > Repository: mesos > > > Description > ------- > > When a framework issues a DESTROY of a shared volume, and that volume > is not in use by a running or a pending task, we rescind the offers > from frameworks to which the shared volume is present in pending offers > so that the volume is not assigned to any task in a future ACCEPT call. > At that time, we need to recover the resources as well for proper > accounting of such resources by the allocator. > > > Diffs > ----- > > src/master/master.cpp 66a672f6d16233e96b29e330a9e6c474546fa851 > src/tests/persistent_volume_tests.cpp > 2ab44d11d159162dfcac9d0791b651ed059b8164 > > Diff: https://reviews.apache.org/r/52288/diff/ > > > Testing > ------- > > All tests passed. > > > Thanks, > > Anindya Sinha > >
