Add option to INFO to return a json payload about kvrocks HOT 13 OPEN

Yeah, and you don't need to parse anything for implementing such behavior.

so is this approach not desirable unstable...VasuDevrani:kvrocks:init_INFO

Yeah, it looks a bit ugly. We can have a more intuitive design for this feature.

from kvrocks.

Yeah, it looks a bit ugly. We can have a more intuitive design for this feature.

i feel confused about the more intuitive design is this close to that 292bdbe? otherwise its out of scope for me (at least for now, will have to wait for someone else's PR or if someone can guide me around the better approach).

I hope you can notice that there is a lot of repetition in your code between if and else, we should avoid this copy-and-paste programming. For me, preventing such code from entering kvrocks codebase is my top priority.

In fact, we can see that the result returned by INFO is a format similar to

std::map<SectionNameTy, std::map<KeyTy, ValueTy>>
(where ValueTy = something like std::variant<int, std::string>)

So we can return this format and transform it into JSON or TEXT at the end.

from kvrocks.

In details, we can have such small functions, like:

auto GetPersistenceInfo() -> std::map<KeyTy, ValueTy>;
auto GetServerInfo() -> std::map<KeyTy, ValueTy>;
auto GetPWhateverInfo() -> std::map<KeyTy, ValueTy>;

And then at the function that can generate all info (not real C++ code):

std::map<..> func_map = {{"persistence", GetPersistenceInfo}, ...}

auto all_info = func_map.filter(selected_info_sections).map(x -> x.call())

if (JSON) generateJsonOutput(all_info)
else (TXT) generateTxtOutput(all_info)

from kvrocks.

do we want INFO command to return output in json fomat when INFO JSON (JSON specified in command) otherwise plain text?

from kvrocks.

I think for compatibility, it should be INFO [<section_name>] [FORMAT (TXT | JSON)].

from kvrocks.

im struggling with parsing the output to json, this line of code outputs string which looks like

"# Clients\r\nmaxclients:10000\r\nconnected_clients:1\r\nmonitor_clients:0\r\nblocked_clients:0\r\n"

using jsoncons::json::parse throws ser_error (unsupported format) and I should not directly append the string as is to the json.

should i create a custom parsing function to convert it something like?
{"clients":{"blocked_clients":"0\r","connected_clients":"1\r","maxclients":"10000\r","monitor_clients":"0\r"}}

from kvrocks.

TBH I cannot get your point. Why do you want to parse it as JSON? It's just not JSON.

from kvrocks.

TBH I cannot get your point. Why do you want to parse it as JSON? It's just not JSON.

Maybe i misunderstood, please confirm.

The issue wants the support of commands INFO [<section_name>] [FORMAT (TXT | JSON)]:

INFO: returns info in default plain text format.
INFO <section>: section info in plain text format.
INFO all FORMAT JSON: returns the entire info in JSON format.
INFO <section> FORMAT JSON: returns just section info in JSON.
INFO <section> FORMAT TXT: section info in plain text format.

from kvrocks.

Yeah, and you don't need to parse anything for implementing such behavior.

from kvrocks.

Yeah, and you don't need to parse anything for implementing such behavior.

so is this approach not desirable
unstable...VasuDevrani:kvrocks:init_INFO

from kvrocks.

Yeah, it looks a bit ugly. We can have a more intuitive design for this feature.

i feel confused about the more intuitive design
is this close to that 292bdbe?
otherwise its out of scope for me (at least for now, will have to wait for someone else's PR or if someone can guide me around the better approach).

from kvrocks.

auto GetPersistenceInfo() -> std::map<KeyTy, ValueTy>;
auto GetServerInfo() -> std::map<KeyTy, ValueTy>;
auto GetPWhateverInfo() -> std::map<KeyTy, ValueTy>;

we already does have some function like GetServerInfo that uses string stream and returns string, should i overwrite them to return std::map<KeyTy, ValueTy>

from kvrocks.

auto GetPersistenceInfo() -> std::map<KeyTy, ValueTy>;
auto GetServerInfo() -> std::map<KeyTy, ValueTy>;
auto GetPWhateverInfo() -> std::map<KeyTy, ValueTy>;

we already does have some function like GetServerInfo that uses string stream and returns string, should i overwrite them to return std::map<KeyTy, ValueTy>

Yeah you can update these methods.

from kvrocks.