----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30694/#review71515 -----------------------------------------------------------
Almost there, are there existing tests that make sure that the volumes get re-offered? If not, let's follow up separately with those. src/master/master.hpp <https://reviews.apache.org/r/30694/#comment117261> It looks like this should be the first thing done in this method, since tasks and executors may be consuming the checkpointed resources. Otherwise, you temporarily make usedResources and totalResources inconcistent, no? src/master/master.cpp <https://reviews.apache.org/r/30694/#comment117262> Hm.. are we not logging the operation in the master? Much like we log the LAUNCH operations, it seems nice to log the other operations. Feel free to do it in the caller since it already has the switch statement. src/tests/persistent_volume_tests.cpp <https://reviews.apache.org/r/30694/#comment117263> This seems too implicit on the resource sizing, can you just set the flag below? It doesn't look like this provides much simpliciation at the cost of the non-local reasoning needed: ``` // Clearly 1024MB for volumes. slave::Flags slaveFlags = CreateSlaveFlags(); slaveFlags.resources = "disk(role1):1024"; Try<PID<Slave>> slave = StartSlave(flags); ``` vs. ``` // How big can my volumes be? Try<PID<Slave>> slave = StartSlave(CreateSlaveFlags("role1")); ``` Is there a reason that the rest (cpu, mem) need to be reserved for these tests? src/tests/persistent_volume_tests.cpp <https://reviews.apache.org/r/30694/#comment117264> s/bytes/size/ ? src/tests/persistent_volume_tests.cpp <https://reviews.apache.org/r/30694/#comment117265> Maybe you want a note that currently we send 1 message per operation? Technically, that's a bit of an implementation detail, we could send 1 per accept :) - Ben Mahler On Feb. 6, 2015, 9:43 p.m., Jie Yu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/30694/ > ----------------------------------------------------------- > > (Updated Feb. 6, 2015, 9:43 p.m.) > > > Review request for mesos, Ben Mahler, Michael Park, and Vinod Kone. > > > Bugs: MESOS-2100 > https://issues.apache.org/jira/browse/MESOS-2100 > > > Repository: mesos > > > Description > ------- > > Fixed bugs in CREATE/DESTROY operation handlers and added tests. > > > Diffs > ----- > > include/mesos/resources.hpp 3b57568c10233a0c692787de6464f21af5eaadf4 > src/Makefile.am 93537d17d3c7604a8532ee1453e405630c481ddc > src/master/master.hpp c3c77f840f089c5754c764e7f150a3c1971d311f > src/master/master.cpp 22fece79c6e511a1b61eb674d7f82216e7a25e00 > src/tests/persistent_volume_tests.cpp PRE-CREATION > > Diff: https://reviews.apache.org/r/30694/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Jie Yu > >
