Add support for basic session management #35
Conversation
include/ppconsul/kv.h
Outdated
| } | ||
|
|
||
| class Sessions |
There was a problem hiding this comment.
It's better to make it in the same way as other endpoints:
- Put it into its own file
sessions.h/sessions.cppunderppconsul::sessionsnamespace. - Constructor takes a reference to
Consulobject rather then toKvone and other applicable default parameters (token, consistency and DC perhaps?).
include/ppconsul/kv.h
Outdated
| } | ||
| }; | ||
|
|
||
| // TODO: add ScopedLock |
include/ppconsul/kv.h
Outdated
| DELETE, | ||
| }; | ||
|
|
||
| struct DestroyError: public ppconsul::Error |
There was a problem hiding this comment.
Why do you need to have a special exception for a mailformed delete session response?
There was a problem hiding this comment.
Should I just ignore the result?
There was a problem hiding this comment.
return parseJsonBool(...) should be fine. The caller can decide what to do in case of false result (probably nothing).
There was a problem hiding this comment.
The fun is, it returns true even if called with non-existing session ID. And I have no idea in what situation it returns false, then.
Nevermind, it can fail because of checks and other stuff.
include/ppconsul/kv.h
Outdated
| class Sessions | ||
| { | ||
| public: | ||
| enum class OnInvalidation |
There was a problem hiding this comment.
Extract from class and name InvalidationBehavior (if placed under sessions namespace, otherwise SessionInvalidationBehavior).
include/ppconsul/kv.h
Outdated
| public: | ||
| enum class OnInvalidation | ||
| { | ||
| RELEASE, |
There was a problem hiding this comment.
Don't use all capital letters for anything that's not a macro. Use CamelCase instead.
include/ppconsul/kv.h
Outdated
| } | ||
|
|
||
| private: | ||
| static std::string encodeBehavior(OnInvalidation behavior) |
include/ppconsul/kv.h
Outdated
| template<class... Params, class = kwargs::enable_if_kwargs_t<Params...>> | ||
| static std::string create(Kv &kv, | ||
| const std::string &node = "", int lockDelay = 15, | ||
| OnInvalidation behavior = OnInvalidation::RELEASE, int ttl = -1, |
There was a problem hiding this comment.
Lock delay and TTL should both be std::chrono::seconds.
include/ppconsul/kv.h
Outdated
| { | ||
| virtual const char *what() const PPCONSUL_NOEXCEPT override | ||
| { | ||
| return "cannot destroy session"; |
|
OK, addressed all the issues. Please review. |
|
This looks great, thank you! |
No description provided.