----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/40247/#review107913 -----------------------------------------------------------
Ship it! Looks good overall! Some nit-picks, some thoughts on naming, etc. docs/persistent-volume.md (line 248) <https://reviews.apache.org/r/40247/#comment167245> It looks pretty clear that we allow the creation and destruction of multiple volumes via these endpoints. Should we call them `create-volumes` and `destroy-volumes` respectively? docs/persistent-volume.md (lines 258 - 259) <https://reviews.apache.org/r/40247/#comment167236> Sorry to be nit-picking, but this still looks like the `-u` and below is 1 space further indented than the `-i`? src/master/http.cpp (line 541) <https://reviews.apache.org/r/40247/#comment167238> I feel like this could be taken as "remove/filter the disk resources" rather than "remove the DiskInfo portion of each resource" :( I thought maybe `removeVolumes` but I think that has the same issue as before. I also think we should keep in mind that we may introduce an alias for, and deprecate `flatten`. Another one would be `removeDiskInfos` to be more indicative that the `DiskInfo` portion of the `Resource`s are being removed, but then the alias for `flatten` would end up as something like, `removeRolesAndReservationInfos`... This brings me to maybe declaring the state in which this resource is being transformed into. Something like... `makeRegularDisk` and `makeUnreserved`? What do you think? src/master/http.cpp (lines 541 - 551) <https://reviews.apache.org/r/40247/#comment167239> As far as its implementation, let's do: ```cpp static Resources removeDisks(Resources resources) { foreach (Resource& resource, resources) { resource.clear_disk(); } return resources; } ``` src/master/http.cpp (lines 614 - 617) <https://reviews.apache.org/r/40247/#comment167240> ```cpp Option<Error> validate = validation::operation::validate( operation.create(), slave->checkpointedResources); ``` or ```cpp Option<Error> validate = validation::operation::validate( operation.create(), slave->checkpointedResources); ``` src/master/http.cpp (lines 704 - 707) <https://reviews.apache.org/r/40247/#comment167241> Similar to above. src/master/http.cpp (line 1046) <https://reviews.apache.org/r/40247/#comment167242> `flatten()` removes `role` and `ReservationInfo` from the resources. src/tests/mesos.hpp (line 520) <https://reviews.apache.org/r/40247/#comment167243> `s/reservationPrinciple/reservationPrincipal/` src/tests/persistent_volume_endpoints_tests.cpp (lines 159 - 162) <https://reviews.apache.org/r/40247/#comment167246> I would suggest that we reorder these since we expect `registered` to occur before `resourceOffers`. Although functionally, it should have no difference. Occurences below as well. src/tests/persistent_volume_endpoints_tests.cpp (line 277) <https://reviews.apache.org/r/40247/#comment167247> We should perhaps use `frameworkInfo.role()` here to make sure that it matches the dynamically reserved role? src/tests/persistent_volume_endpoints_tests.cpp (line 382) <https://reviews.apache.org/r/40247/#comment167248> Would the following serve as good documentation of the test? ``` ASSERT_NE(frameworkInfo.role(), "role2"); ``` src/tests/persistent_volume_endpoints_tests.cpp (line 443) <https://reviews.apache.org/r/40247/#comment167249> This also needs to match the dynamically reserved role, right? `frameworkInfo.role()`? - Michael Park On Nov. 23, 2015, 5:06 p.m., Neil Conway wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/40247/ > ----------------------------------------------------------- > > (Updated Nov. 23, 2015, 5:06 p.m.) > > > Review request for mesos, Greg Mann and Michael Park. > > > Bugs: MESOS-2455 > https://issues.apache.org/jira/browse/MESOS-2455 > > > Repository: mesos > > > Description > ------- > > Added HTTP endpoints for creating and destroying persistent volumes. > > > Diffs > ----- > > docs/persistent-volume.md 0951ccb69daaa19b959e11cf3bf972a674a58305 > docs/reservation.md 81f21c3755b216b0932876c1ddd9de4d3fbe814a > src/Makefile.am 8d14ff803249b5b81b696d40d37e013960dee41b > src/master/http.cpp 1c4f1406f5d917f5d655a7d61d311365f8999ce0 > src/master/master.hpp d4b1edde98925fd51e056f253758afea779be9ed > src/master/master.cpp d2bc83cd77ae7fe723ccb35a7c1e0b70a04a0d6e > src/tests/mesos.hpp b3f69ccb9870b17a335a2fe7dbf2802c1b709e6b > src/tests/persistent_volume_endpoints_tests.cpp PRE-CREATION > > Diff: https://reviews.apache.org/r/40247/diff/ > > > Testing > ------- > > (1) make check, including newly added tests > > (2) Manually created/removed persistent volumes via HTTP endpoints + curl. > > (3) Previewed docs in Github gist. > > > Thanks, > > Neil Conway > >
