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 Wed, 25 Nov 2015 21:15:52 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;
> >     }
> >     ```

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.


- Neil


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


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


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