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

refactor: add Configurable trait #3917

Merged
merged 10 commits into from
May 15, 2024

Conversation

zyy17
Copy link
Collaborator

@zyy17 zyy17 commented May 11, 2024

I hereby agree to the terms of the GreptimeDB CLA.

Refer to a related PR or issue link (optional)

What's changed and what's your intention?

Add the Configurable trait to make configuration more general.

  1. Add the new module error.rs and config.rs in common/config. Most of the code are from cmd/:

    • The load_layer_options() is from the original cmd/options.rs;
    • The errors are from cmd/error.rs;
  2. Add merge_with_cli_options() to make load_options() more simple;

  3. Implement Configurable trait and changed some tests for {DatanodeOptions, FrontendOptions, MetasrvOptions, StandaloneOptions};

Checklist

  • I have written the necessary rustdoc comments.
  • I have added the necessary unit tests and integration tests.
  • This PR does not require documentation updates.

@github-actions github-actions bot added the docs-not-required This change does not impact docs. label May 11, 2024
@zyy17 zyy17 force-pushed the refactor/add-configurable-trait branch from 56a1842 to 67f0445 Compare May 11, 2024 08:56
@zyy17 zyy17 marked this pull request as ready for review May 11, 2024 08:56
@zyy17 zyy17 requested review from MichaelScofield and a team as code owners May 11, 2024 08:56
@zyy17 zyy17 requested review from evenyag and removed request for a team May 11, 2024 08:57
Copy link

codecov bot commented May 11, 2024

Codecov Report

Attention: Patch coverage is 95.29915% with 11 lines in your changes are missing coverage. Please review.

Project coverage is 85.47%. Comparing base (15d7b97) to head (e6ec8ee).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3917      +/-   ##
==========================================
- Coverage   85.77%   85.47%   -0.30%     
==========================================
  Files         968      969       +1     
  Lines      166463   166503      +40     
==========================================
- Hits       142782   142319     -463     
- Misses      23681    24184     +503     

@zyy17 zyy17 force-pushed the refactor/add-configurable-trait branch 2 times, most recently from a8c1f71 to ef75272 Compare May 12, 2024 09:20
src/cmd/src/error.rs Show resolved Hide resolved
src/datanode/src/error.rs Show resolved Hide resolved
src/frontend/src/error.rs Show resolved Hide resolved
@zyy17 zyy17 marked this pull request as draft May 13, 2024 13:05
@zyy17 zyy17 force-pushed the refactor/add-configurable-trait branch from ef75272 to f54c812 Compare May 14, 2024 10:30
@zyy17 zyy17 marked this pull request as ready for review May 14, 2024 11:03
@zyy17 zyy17 force-pushed the refactor/add-configurable-trait branch from b8971b8 to d53366a Compare May 14, 2024 11:20
@zyy17 zyy17 requested a review from evenyag May 14, 2024 11:35
@zyy17 zyy17 force-pushed the refactor/add-configurable-trait branch from d53366a to bed8602 Compare May 14, 2024 13:43
@zyy17 zyy17 mentioned this pull request May 14, 2024
3 tasks
src/common/config/src/config.rs Outdated Show resolved Hide resolved
@zyy17 zyy17 force-pushed the refactor/add-configurable-trait branch from bed8602 to e6ec8ee Compare May 15, 2024 03:12
@zyy17
Copy link
Collaborator Author

zyy17 commented May 15, 2024

@evenyag PTAL

@evenyag evenyag added this pull request to the merge queue May 15, 2024
Merged via the queue into GreptimeTeam:main with commit 63a8d29 May 15, 2024
21 checks passed
@zyy17 zyy17 deleted the refactor/add-configurable-trait branch May 15, 2024 13:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs-not-required This change does not impact docs.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants