You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Cody Maloney <co...@mesosphere.io> on 2015/02/04 01:09:22 UTC
Re: Review Request 30394: etcd api wrapper


> On Jan. 29, 2015, 3:40 p.m., Alexander Rukletsov wrote:
> > src/etcd/url.hpp, line 23
> > <https://reviews.apache.org/r/30394/diff/1/?file=839637#file839637line23>
> >
> >     `#include <iostream>`
> >     or, what I would prefer
> >     `#include <iosfwd>`
> >     and move `operator<<` definitions into `url.cpp`

Strings already includes <iostream> so it's not necessary here. From scanning the codebase the standard pattern currently is to define these inline in the headers from a scan of the codebase.


> On Jan. 29, 2015, 3:40 p.m., Alexander Rukletsov wrote:
> > src/etcd/url.cpp, line 18
> > <https://reviews.apache.org/r/30394/diff/1/?file=839638#file839638line18>
> >
> >     Shouldn't we add `<cassert>`?

I'm following the pattern of zookeeper here which doesn't do it. The code already errors if an empty string is passed (with a runtime error, which it is rather than a programmer logic error which is an assert), and the codebase doesn't assert references are non-null (even though they can be), so I don't see where I should add asserts.


- Cody


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


On Jan. 29, 2015, 3:58 a.m., Cody Maloney wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30394/
> -----------------------------------------------------------
> 
> (Updated Jan. 29, 2015, 3:58 a.m.)
> 
> 
> Review request for mesos and Benjamin Hindman.
> 
> 
> Bugs: MESOS-1806
>     https://issues.apache.org/jira/browse/MESOS-1806
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> etcd api wrapper
> 
> 
> Diffs
> -----
> 
>   src/Makefile.am 07bea1fb8f0035413f2119859e16fa4f9383f68e 
>   src/etcd/etcd.hpp PRE-CREATION 
>   src/etcd/etcd.cpp PRE-CREATION 
>   src/etcd/url.hpp PRE-CREATION 
>   src/etcd/url.cpp PRE-CREATION 
>   src/master/constants.hpp c386eab3973363e64c113f7e84452456fdd3393c 
>   src/master/constants.cpp 9ee17e9ad884da98cabfdfe316cb4a831de193b3 
> 
> Diff: https://reviews.apache.org/r/30394/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Cody Maloney
> 
>