oliora / ppconsul

C++ client for Consul (http://consul.io)
Boost Software License 1.0
152 stars 56 forks source link

Add typed interface for KV store #60

Closed indev29 closed 4 years ago

indev29 commented 4 years ago

This PR adds typed interface for KV methods making it possible to call get<double>(key) and set(key, some_double) using boost::lexical_cast. It is also possible to pass user classes if they are Input/Output Streamable. KeyValue struct still stores string value but now has a get<T>() method. CAS and lock/unlock methods are also implemented, transactions are still string value only.

There are also some (honestly, random) tests.

oliora commented 4 years ago

Hi indev29,

I really appreciate your effort for this PR but I'm not sure that ppconsul should provide a typed KV-store interface. And even if ppconsul provides it, the solution should not be based on boost::lexical_cast.

The main reason why I don't want ppconsul to have it because there is no standard way of how values of different types are represented as Consul values. One is highly varied between users thus it's hard to provide a solution that's good for most of the customers. Especially if we're talking about a generic solution that ideally should support any user type.

boost::lexical_cast does not help here because it does not provide a good way of extending it except by using IOstream operator which is usually already used for another purpose like formatting things for non-critical human consumption and also has a debatable performance. Any type that is not an integer is barely supported by boost::lexical_cast. My immediate concerns are: what's a floating point precision on saving? How booleans are represented? How can I set and get a binary value or a collection? Then there are questions about interoperability in case if Consul's KV store is used not only from ppconsul but from applications written in different languages.

I think that if ppconsul ever provides a typed interface for KV-store it should only support primitive types that are supported by JSON (i.e. string, number and bool) and represent them inside string as a JSON value would be. Everything outside of this should be left up to a user. Users can build any serialization layer on top of the string value if they want to.

UPD: Next time, please create an issue first where we can discuss about whether it is needed and how this should be implemented. This may save our time and perhaps allow to come up with a better implementation approach.

indev29 commented 4 years ago

Thanks for the detailed response, I see your point. I actually found this one in status.md (which probably should be updated) and decided to implement it. But yeah, creating an issue first is a right way to go.

I'd like to contribute more, so it would be nice if you created a couple more issues with the tasks that you think are important for the project right now, because I'm still quite new to the library and Consul itself.

oliora commented 4 years ago

Yes, when I worked on the KV-store (quite some time ago) I thought that it should provide a typed interface. I'm much less sure about this now. I mean, the typed interface is a good thing but making one that does not suck is hard and I don't want to paint ppconsul users in the corner with a solution that is likely to be not good enough in long term.

I think, supporting more Consul endpoints/features is the most important direction to work on - you're more than welcome to work on this. Other important features are: #3, #52 and #26 (this one needs a design and perhaps better to be done after migrating ppconsul to a newer C++ standard). I also want to migrate Ppconsul to C++17 or 20 and replace boost::optional, boost::variant and possibly some other Boost components with the standard library constructs. I also want to rewrite keyword arguments with Boost.MP11 (currently they're implemented with Boost.MPL and Boost.Fusion) to get better compilation time and simpler code. I'll create missing issues soon.