> 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
>
>
