lucid-kv / lucid

High performance and distributed KV store w/ REST API. 🦀
https://clintnetwork.gitbook.io/lucid/
MIT License
378 stars 31 forks source link

Add comments to documentation #22

Closed imclint21 closed 4 years ago

imclint21 commented 4 years ago

I think it could be cool to have comments in documentation!

# When diskless replication is used, the master waits a configurable amount of
# time (in seconds) before starting the transfer in the hope that multiple replicas
# will arrive and the transfer can be parallelized.
#
# With slow disks and fast (large bandwidth) networks, diskless replication
# works better.
repl-diskless-sync no

Issue Opened on serde_yaml Github: https://github.com/dtolnay/serde-yaml/issues/145

shuni64 commented 4 years ago

While I like this idea, I'm not sure if this is easily possible to do in code, even if it is it doesn't seem like a good idea to hardcode documentation as strings in the binary.

Instead I suggest keeping a default configuration file with comments in the repository and packaging it with releases. The authentication tokens and keys would have to be stored somewhere else, but maybe this should be the case anyway.

This also gives us the opportunity rethink the idea of explicitly initializing the configuration file through lucid. If a default configuration is packaged with releases there shouldn't be a need to create one when running Lucid for the first time. The only thing that should have to be generated is authentication data, because it doesn't have a good default.

I also had another thought when looking at the way configuration is handled: The server and the interactive cli are the same binary. Maybe this should not be the case, and all settings changes by the cli should have to go through appropriate api endpoints on the server. This way the server and the cli are decoupled and don't have to run on the same machine, which has long-term benefits like being able to temporarily change a setting without restarting the server and allowing other programs to change settings with api requests. It would also clean up the code quite a bit, as most of the cli handling would be gone from the server.

imclint21 commented 4 years ago

It's a good idea on the paper, just some points are important.

Lucid is build to be a DevOps software, running with some Linux standards, with /etc/lucid/lucid.yml for configuration and /var/log/lucid.yml for logging by example.

So pack a default configuration file in the release zip is not a bad idea but we also need to keep the init command to have both possibilities in my opinion.

If we deploy Lucid on Linux package managers maybe we could add default configuration with comments and not in the GitHub releases.

The init command is important for me to bootstrap the Lucid instance very quickly, some programs like consul etc don't have this and it's really boring to configure it.


About the CLI, I want to keep a single binary BUT you right on many points!

Firstly, if you look at this line, the actual CLI is thought (but not coded) as a way to manage lucid nodes, not only the local node but each node, maybe we could make a new API endpoint like /api/settings/ to make hot changes on the node (and also adding CLI commands)

shuni64 commented 4 years ago

Keeping the init subcommand is fine, it's just that comments won't be included when the configuration is written unless the default configuration file is compiled into the binary with include_str!, but even then any changes to it would have to be written without comments. We could just not have an api to permanently change the configuration on disk and require the user to change the files themselves, which should be fine.


Splitting Lucid into server and cli binaries isn't necessary to implement any feature, but would decouple two parts of Lucid that shouldn't directly interact with each other (only using api endpoints), so it's mainly for code quality reasons. I think I'll test some of this on my fork to see if it would bring significant enough improvements to be worth it.

imclint21 commented 4 years ago

The serde_yaml guys don't want to implement comments support in the lib, and a hardcoded configuration file is definitely a bad idea, so I will close this issue I think!

For the CLI and the server decoupled, I'm really for it and it's the idea [from the begining], in fact, the CLI is only an HTTP client.

Feel free to achieve this, If you use an HTTP library maybe you could don't use reqwest, it's a bit heavy (~4 MB more for the binary)