> On Nov. 25, 2015, 5:01 a.m., Michael Park wrote: > > src/master/http.cpp, lines 541-551 > > <https://reviews.apache.org/r/40247/diff/6/?file=1137470#file1137470line541> > > > > As far as its implementation, let's do: > > > > ```cpp > > static Resources removeDisks(Resources resources) > > { > > foreach (Resource& resource, resources) { > > resource.clear_disk(); > > } > > return resources; > > } > > ``` > > Neil Conway wrote: > Is this really better? You C++ guys are so tricky with your value > semantics :) > > To me, the version in the review makes it more obvious that we are > copying the input value, mutating the copy, and then returning the copy. > Whereas in your version, at first glance the reader might think the function > modifies its input value in place. > > What do you think? Happy to make the change if you think it is an > improvement.
Well, I suppose "better" is subjective. It's more efficient, but I didn't think (and don't think) that this is less readable. But that may be just me. Also, the code that fits your description would be: ```cpp static Resources removeDisks(const Resources& resources) { Resources result = resources; // copying the input value. // mutating the copy. foreach (Resource& resource, result) { resource.clear_disk(); } return result; // returning the copy. } ``` This version has half as many copies as the one in the review, and (I think) is also just as readable. So you have 3 options, I'll leave it upto you :) > On Nov. 25, 2015, 5:01 a.m., Michael Park wrote: > > src/master/http.cpp, line 541 > > <https://reviews.apache.org/r/40247/diff/6/?file=1137470#file1137470line541> > > > > 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? > > Neil Conway wrote: > I vote for `removeDiskInfos()`, since it is an improvement. If/when we > want to rename `flatten()` we can always revisit this -- since > `removeDiskInfos` is private anyway, it should be easy to rename. Sounds good! > On Nov. 25, 2015, 5:01 a.m., Michael Park wrote: > > src/tests/persistent_volume_endpoints_tests.cpp, lines 159-162 > > <https://reviews.apache.org/r/40247/diff/6/?file=1137474#file1137474line159> > > > > 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. > > Neil Conway wrote: > Sounds good. Actually I just copied this code from > reservation_endpoints_test.cpp :) So I'll fix similar code there, in a > separate review. Oh, that's embarrassing/great! Thanks :) - Michael ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/40247/#review107913 ----------------------------------------------------------- On Nov. 25, 2015, 10:21 p.m., Neil Conway wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/40247/ > ----------------------------------------------------------- > > (Updated Nov. 25, 2015, 10:21 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 0c9e0e93c3ae8e00f841303e8c2b26b36a775eac > docs/reservation.md e297921e709838a6780c58a12637b261fa7f18cb > src/Makefile.am de68e24fb2ad4c6e4175fbf8658b23bc6434a356 > src/master/http.cpp cffd20b2557b84b415940ab9af8d374c71b6627b > src/master/master.hpp e89c11a1709f0c94c1f5fb988e71d081900a4325 > src/master/master.cpp 92380952277ae3fe0b535718b6b1b8732e960745 > src/tests/mesos.hpp eabbf44626b0e14d93febb55b1b22c9ad236daa1 > 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 > >