mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Neil Conway" <neil.con...@gmail.com>
Subject Re: Review Request 40247: Added HTTP endpoints for creating and destroying persistent volumes.
Date Thu, 26 Nov 2015 21:26:41 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.
> 
> Michael Park wrote:
>     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 :)

Cool -- I went with the last version (copy input and then mutate the copy in-place). Thanks
for the advice! :)


- Neil


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40247/#review107913
-----------------------------------------------------------


On Nov. 26, 2015, 9:26 p.m., Neil Conway wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40247/
> -----------------------------------------------------------
> 
> (Updated Nov. 26, 2015, 9:26 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 0bb315a16801de9e7014ca0a83c5152faa75eb38 
>   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