rough and simplistic etcd client

rough and simplistic etcd client

[Pierre Phaneuf (Google), 2014-10-09 16:58:23]

See also: https://github.com/google/certificate-transparency/pull/175

[Ben Laurie (Google), 2014-10-10 10:58:06]

https://codereview.appspot.com/156040043/diff/20001/cpp/util/etcd.cc
File cpp/util/etcd.cc (right):

https://codereview.appspot.com/156040043/diff/20001/cpp/util/etcd.cc#newcode40
cpp/util/etcd.cc:40: class EtcdClientImpl : public EtcdClient {
Why is this subclass needed?

https://codereview.appspot.com/156040043/diff/20001/cpp/util/etcd.cc#newcode55
cpp/util/etcd.cc:55: typedef map<pair<string, unsigned short>,
Throughout, you mean uint16_t, not unsigned short, right?

https://codereview.appspot.com/156040043/diff/20001/cpp/util/etcd.cc#newcode81
cpp/util/etcd.cc:81: EtcdClientImpl* const self_;
|self_| is a pretty odd name for something that actually isn't yourself.

|client_| perhaps?

https://codereview.appspot.com/156040043/diff/20001/cpp/util/etcd.h
File cpp/util/etcd.h (right):

https://codereview.appspot.com/156040043/diff/20001/cpp/util/etcd.h#newcode32
cpp/util/etcd.h:32: virtual void Generic(const std::string& key,
It also should have a name that means something.

[Pierre Phaneuf (Google), 2014-10-10 11:10:51]

https://codereview.appspot.com/156040043/diff/20001/cpp/util/etcd.cc
File cpp/util/etcd.cc (right):

https://codereview.appspot.com/156040043/diff/20001/cpp/util/etcd.cc#newcode40
cpp/util/etcd.cc:40: class EtcdClientImpl : public EtcdClient {
On 2014/10/10 10:58:06, Ben Laurie (Google) wrote:
> Why is this subclass needed?

I like to hide icky implementations if I can, and I'll be having a
MockEtcdClient, which works best from an interface.

If there wasn't going to be an "alternative" implementation (the mock one), I
might have used a "pimpl", but there is, so I went with an interface and a
hidden implementation.

https://codereview.appspot.com/156040043/diff/20001/cpp/util/etcd.cc#newcode55
cpp/util/etcd.cc:55: typedef map<pair<string, unsigned short>,
On 2014/10/10 10:58:06, Ben Laurie (Google) wrote:
> Throughout, you mean uint16_t, not unsigned short, right?

I lifted the type that libevent's evhttp_connection_base_new takes directly. I
can switch this file to uint16_t, sure.

https://codereview.appspot.com/156040043/diff/20001/cpp/util/etcd.cc#newcode81
cpp/util/etcd.cc:81: EtcdClientImpl* const self_;
On 2014/10/10 10:58:03, Ben Laurie (Google) wrote:
> |self_| is a pretty odd name for something that actually isn't yourself.
> 
> |client_| perhaps?

Totally right, this got moved around a few times, and it used to make more
sense. :-)

I'll rename it.

[Ben Laurie (Google), 2014-10-10 11:31:15]

LGTM.

https://codereview.appspot.com/156040043/diff/20001/cpp/util/etcd.cc
File cpp/util/etcd.cc (right):

https://codereview.appspot.com/156040043/diff/20001/cpp/util/etcd.cc#newcode55
cpp/util/etcd.cc:55: typedef map<pair<string, unsigned short>,
On 2014/10/10 11:10:51, Pierre Phaneuf (Google) wrote:
> On 2014/10/10 10:58:06, Ben Laurie (Google) wrote:
> > Throughout, you mean uint16_t, not unsigned short, right?
> 
> I lifted the type that libevent's evhttp_connection_base_new takes directly. I
> can switch this file to uint16_t, sure.

File a bug against libevent (coz it is a bug).

[Pierre Phaneuf (Google), 2014-10-10 11:59:03]

On 2014/10/10 11:31:15, Ben Laurie (Google) wrote:
> LGTM.
> 
> https://codereview.appspot.com/156040043/diff/20001/cpp/util/etcd.cc
> File cpp/util/etcd.cc (right):
> 
> https://codereview.appspot.com/156040043/diff/20001/cpp/util/etcd.cc#newcode55
> cpp/util/etcd.cc:55: typedef map<pair<string, unsigned short>,
> On 2014/10/10 11:10:51, Pierre Phaneuf (Google) wrote:
> > On 2014/10/10 10:58:06, Ben Laurie (Google) wrote:
> > > Throughout, you mean uint16_t, not unsigned short, right?
> > 
> > I lifted the type that libevent's evhttp_connection_base_new takes directly.
I
> > can switch this file to uint16_t, sure.
> 
> File a bug against libevent (coz it is a bug).

Interesting, I thought it was still "unsigned short", but my
/usr/include/net/in.h indeed uses uint16_t (through an in_port_t typedef)... On
Mac, it's __uint16_t, but pretty much the same. I'm not 100% sure on how
unsigned short is a bug ("that's what I grew up with!"), and there might be API
backward compatibility issues, but I'll point this out to them.

I'm guessing that's what OpenBSD still uses, you know Niels... ;-)

[Ben Laurie (Google), 2014-10-16 15:23:41]

On Fri Oct 10 2014 at 12:59:03 PM <pphaneuf@google.com> wrote:

> On 2014/10/10 11:31:15, Ben Laurie (Google) wrote:
> > LGTM.
>
> > https://codereview.appspot.com/156040043/diff/20001/cpp/util/etcd.cc
> > File cpp/util/etcd.cc (right):
>
>
> https://codereview.appspot.com/156040043/diff/20001/cpp/
> util/etcd.cc#newcode55
> > cpp/util/etcd.cc:55: typedef map<pair<string, unsigned short>,
> > On 2014/10/10 11:10:51, Pierre Phaneuf (Google) wrote:
> > > On 2014/10/10 10:58:06, Ben Laurie (Google) wrote:
> > > > Throughout, you mean uint16_t, not unsigned short, right?
> > >
> > > I lifted the type that libevent's evhttp_connection_base_new takes
> directly. I
> > > can switch this file to uint16_t, sure.
>
> > File a bug against libevent (coz it is a bug).
>
> Interesting, I thought it was still "unsigned short", but my
> /usr/include/net/in.h indeed uses uint16_t (through an in_port_t
> typedef)... On Mac, it's __uint16_t, but pretty much the same. I'm not
> 100% sure on how unsigned short is a bug ("that's what I grew up
> with!"),


OK, I guess whether its a bug could be debated - ports are 16 bits, shorts
are 16 bits or more. Forcing people to burn more storage than they need to
seems like a bug to me.


> and there might be API backward compatibility issues, but I'll
> point this out to them.
>
> I'm guessing that's what OpenBSD still uses, you know Niels... ;-)
>
> https://codereview.appspot.com/156040043/
>