Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(config-endpoint): add initial implementation #1896

Merged
merged 11 commits into from Jul 11, 2023
Merged

feat(config-endpoint): add initial implementation #1896

merged 11 commits into from Jul 11, 2023

Conversation

etolbakov
Copy link
Collaborator

@etolbakov etolbakov commented Jul 6, 2023

I hereby agree to the terms of the GreptimeDB CLA

What's changed and what's your intention?

add /config API endpoint to retrieve the current configuration(Options) that were used for the app to start

Please explain IN DETAIL what the changes are in this PR and why they are needed:

Checklist

  • I have written the necessary rustdoc comments.
  • I have added the necessary unit tests and integration tests.

Refer to a related PR or issue link

#1793

@etolbakov
Copy link
Collaborator Author

Hello @killme2008 @sunng87 @MichaelScofield @waynexia
Could I please have your feedback on the approach that I took for this task?

I understand the main challenge is to pass the Options enum to the http.
My idea is to use a file system for info exchange, particularly, write Options on start into /tmp/greptimedb/conf/opts.json as an example (hardcoded at the moment).
When it comes to /config API invocation, it will boil down to reading a file from that directory.

  1. Does this approach seem fine for you and should I proceed? Please let me know if there's a better way of doing that?
  2. In the original describption the desired output format was toml. Do we consider using json?

@waynexia
Copy link
Member

waynexia commented Jul 6, 2023

In the original describption the desired output format was toml. Do we consider using json?

I prefer toml because it's the format we (the binary) accept 🧐

@MichaelScofield
Copy link
Collaborator

@etolbakov I think /config endpoint could just return plain text in http, does not need to use an intermediate file to exchange data. As to the output format, I strongly suggest toml. So that user could copy the output from /config endpoint and paste them in another GreptimeDB, very convenient, very ergonomic.

@waynexia
Copy link
Member

waynexia commented Jul 6, 2023

I think /config endpoint could just return plain text in http, does not need to use an intermediate file to exchange data.

This config file can be seen as another kind of log. Just like the config printed on start but this file won't go outdated. So I'm ok with this temp file

@etolbakov
Copy link
Collaborator Author

Thank you very much for the feedback 👍!

Michael, sorry for confusion. As one remarkable programmer told - "Talk is cheap, show me the code", so I put a bit more details to support my initial idea. Could you please take a look when you have time?

Here's how it works at the moment. The created file:

❯ ls -lah /tmp/greptimedb/conf
drwxr-xr-x  3 etolbakov  wheel    96B  6 Jul 14:45 .
drwxr-xr-x  8 etolbakov  wheel   256B  6 Jul 15:04 ..
-rw-r--r--  1 etolbakov  wheel   1.8K  6 Jul 15:04 opts.toml    

The app start log:

❯ /target/release/greptime standalone start
2023-07-06T14:04:26.845441Z  INFO greptime: short_version: 0.3.2, full_version: greptimedb-feat/http-config-endpoint-c60c695
2023-07-06T14:04:26.845543Z  INFO greptime: command line arguments
2023-07-06T14:04:26.845547Z  INFO greptime: argument: ./target/release/greptime
2023-07-06T14:04:26.845559Z  INFO greptime: argument: standalone
2023-07-06T14:04:26.845561Z  INFO greptime: argument: start
2023-07-06T14:04:26.846407Z  INFO greptime: Configuration options were successfully written at '/tmp/greptimedb/conf/opts.toml'
2023-07-06T14:04:26.846501Z  INFO cmd::standalone: Standalone start command: StartCommand {
    http_addr: None,
.... <redacted>

Endpoint invocation example:

❯ curl  http://127.0.0.1:4000/config
type = "Standalone"
type = "MixOptions"

[fe_opts]
mode = "standalone"
heartbeat_interval_millis = 5000
retry_interval_millis = 5000

[fe_opts.http_options]
addr = "127.0.0.1:4000"
timeout = "30s"
body_limit = "64MiB"
.... <redacted>

Happy to proceed with adding tests and putting it under admin api if there are no major reservations with this approach.

src/cmd/src/bin/greptime.rs Outdated Show resolved Hide resolved
@MichaelScofield
Copy link
Collaborator

@etolbakov I see, so you are generating a static file from config at the start of running GreptimeDB, and reading it back to user in http text when /config endpoint is called. I'm ok with that. However, is it simpler to just store the config struct in http service, then upon be called /config, dynamically render it to http text and return? The frequency of calling endpoint /config won't be too much, and the convertion (config struct -> toml file format) should be cheap, so I think the cost is fine.

@etolbakov
Copy link
Collaborator Author

@MichaelScofield, thank you very much for the feedback!
Your comment made me to review possible options again, I think I came up with slightly better approach.
Could you please take a look when you have time?

src/servers/src/http.rs Outdated Show resolved Hide resolved
@etolbakov etolbakov marked this pull request as ready for review July 10, 2023 10:22
@etolbakov
Copy link
Collaborator Author

@MichaelScofield @waynexia
Could you please take a look when you have a spare minute?

Copy link
Member

@waynexia waynexia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good, thanks

tests-integration/tests/http.rs Show resolved Hide resolved
@MichaelScofield MichaelScofield merged commit fc850c9 into GreptimeTeam:develop Jul 11, 2023
10 checks passed
@etolbakov etolbakov deleted the feat/http-config-endpoint branch July 11, 2023 06:23
paomian pushed a commit to paomian/greptimedb that referenced this pull request Oct 19, 2023
* feat(config-endpoint): add initial implementation

* feat: add initial handler implementation

* fix: apply clippy suggestions, use axum response instead of string

* feat: address CR suggestions

* fix: minor adjustments in formatting

* fix: add a test

* feat: add to_toml_string method to options

* fix: adjust the assertion for the integration test

* fix: adjust expected indents

* fix: adjust assertion for the integration test

* fix: improve according to clippy
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants