mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Michael Park" <mcyp...@gmail.com>
Subject Re: Review Request 40247: Added HTTP endpoints for creating and destroying persistent volumes.
Date Thu, 26 Nov 2015 00:32:37 GMT


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


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message