Skip to content

refactor: respect data_home as root data home directory #6050

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

zyy17
Copy link
Collaborator

@zyy17 zyy17 commented May 6, 2025

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?

respect data_home as root data home directory:

  1. When logs_dir is not specified, use ${data_home}/${logs_dir} as default logs dir;
  2. Rename METASRV_HOME to METASRV_DATA_DIR and change its default value to metasrv/. The actual metasrv_home will be ${data_home}/${METASRV_DATA_DIR};
  3. Normalize some constants about *DIR;

PR Checklist

Please convert it to a draft if some of the following conditions are not met.

  • I have written the necessary rustdoc comments.
  • I have added the necessary unit tests and integration tests.
  • This PR requires documentation updates.
  • API changes are backward compatible.
  • Schema or data changes are backward compatible.

@github-actions github-actions bot added the docs-not-required This change does not impact docs. label May 6, 2025
@zyy17 zyy17 force-pushed the refactor/respect-data-home-in-logging-options branch from 118b05d to c863e3a Compare May 29, 2025 07:47
@zyy17 zyy17 marked this pull request as ready for review May 29, 2025 08:44
@zyy17 zyy17 requested review from MichaelScofield and a team as code owners May 29, 2025 08:44
@zyy17 zyy17 force-pushed the refactor/respect-data-home-in-logging-options branch from c863e3a to 5cf1b1c Compare May 29, 2025 12:27
@zyy17 zyy17 changed the title refactor: initialize logging dir by using data_home refactor: respect data_home as root data home directory May 29, 2025
@zyy17 zyy17 requested review from fengjiachun, Copilot and sunng87 and removed request for a team May 29, 2025 12:50
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR centralizes path configuration by introducing DEFAULT_DATA_HOME as the root data directory, renaming and refactoring component‐specific home and log directories to use this base, and updating code, tests, and example configs to join subdirectories under data_home.

  • Introduce DEFAULT_DATA_HOME and DEFAULT_LOGGING_DIR constants and remove hardcoded paths
  • Rename METASRV_HOME to METASRV_DATA_DIR and update MetasrvOptions defaults
  • Update CLI commands to set logging.dir under data_home when unset and refresh example configs/docs

Reviewed Changes

Copilot reviewed 17 out of 17 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
src/meta-srv/src/metasrv/builder.rs Build metasrv_home by joining data_home and METASRV_DATA_DIR
src/meta-srv/src/metasrv.rs Rename METASRV_HOMEMETASRV_DATA_DIR, use DEFAULT_DATA_HOME
src/datanode/src/config.rs Remove local DEFAULT_DATA_HOME, import global constant
src/common/telemetry/src/logging.rs Add DEFAULT_LOGGING_DIR, clear default dir for runtime override
src/common/config/src/lib.rs Add DEFAULT_DATA_HOME constant
src/cmd/tests/load_config_test.rs Update tests to use DEFAULT_DATA_HOME/DEFAULT_LOGGING_DIR
src/cmd/src/{standalone,metasrv,frontend,flownode,datanode}.rs Set logging.dir to <data_home>/logs/ if empty
config/*.example.toml, config/config.md Refresh example paths with trailing slashes and new defaults
Comments suppressed due to low confidence (2)

src/cmd/tests/load_config_test.rs:61

  • Example configs now use a trailing slash for the WAL directory, but WAL_DIR may not include it. Ensure WAL_DIR constant includes a trailing slash or update the test to expect the trailing slash so the loaded value matches the TOML example.
Path::new(DEFAULT_DATA_HOME).join(WAL_DIR).to_string_lossy().to_string(),

src/cmd/src/standalone.rs:408

  • [nitpick] Consider adding a unit or integration test to verify that logging.dir is correctly set to <data_home>/logs/ when the option is left empty, to prevent regressions in this new fallback logic.
if opts.logging.dir.is_empty() {

@zyy17 zyy17 requested a review from v0y4g3r as a code owner May 29, 2025 14:11
@zyy17 zyy17 requested a review from Copilot May 29, 2025 14:11
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR refactors directory configurations to respect data_home as the root data directory by updating default constants and configuration paths. Key changes include:

  • Using data_home to compute default paths for logs, WAL, and metasrv directories.
  • Renaming METASRV_HOME to METASRV_DATA_DIR for clarity.
  • Normalizing configuration files and tests to align with the new default directory constants.

Reviewed Changes

Copilot reviewed 16 out of 16 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
src/store-api/src/path_utils.rs Normalizes WAL_DIR by removing the trailing slash.
src/meta-srv/src/metasrv/builder.rs Updates metasrv home construction using data_home and METASRV_DATA_DIR.
src/meta-srv/src/metasrv.rs Replaces METASRV_HOME with METASRV_DATA_DIR and updates logging defaults.
src/datanode/src/config.rs Leverages DEFAULT_DATA_HOME from common_config.
src/common/telemetry/src/logging.rs Updates LoggingOptions default directory configuration.
src/common/config/src/lib.rs Adds DEFAULT_DATA_HOME constant.
src/cmd/tests/load_config_test.rs Aligns test configurations with updated default directory constants.
src/cmd/src/standalone.rs Sets logging directory using data_home if not provided.
src/cmd/src/metasrv.rs Sets logging directory using data_home if unset.
src/cmd/src/frontend.rs Sets logging directory using data_home if unset.
src/cmd/src/flownode.rs Sets logging directory using data_home if unset.
src/cmd/src/datanode.rs Sets logging directory using data_home if unset.
config/standalone.example.toml Normalizes storage.data_home configuration.
config/metasrv.example.toml Normalizes data_home configuration.
config/datanode.example.toml Normalizes storage.data_home configuration.
config/config.md Updates documentation to reflect new data_home defaults.
Comments suppressed due to low confidence (1)

src/store-api/src/path_utils.rs:21

  • Removing the trailing slash from WAL_DIR may affect path concatenations if downstream consumers expect a trailing separator. Verify that all usages correctly handle this change or adjust the constant or consumers accordingly.
pub const WAL_DIR: &str = "wal";

@zyy17 zyy17 force-pushed the refactor/respect-data-home-in-logging-options branch from 46c2019 to 72d4897 Compare May 29, 2025 15:40
@@ -314,7 +314,7 @@

| Key | Type | Default | Descriptions |
| --- | -----| ------- | ----------- |
| `data_home` | String | `./greptimedb_data/metasrv/` | The working home directory. |
| `data_home` | String | `./greptimedb_data` | The working home directory. |
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this a breaking change if users are using the default configuration?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's not a breaking change.

In the original implementation, it uses the following code to create a data home for metasrv:

let metasrv_home = options.data_home.to_string();

This PR change to(METASRV_DATA_DIR is metasrv):

        let metasrv_home = Path::new(&options.data_home)
            .join(METASRV_DATA_DIR)
            .to_string_lossy()
            .to_string();

The final config is the same.

Copy link
Collaborator Author

@zyy17 zyy17 May 30, 2025

Choose a reason for hiding this comment

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

After some thought, it's not breaking change for default configs. However, it could be a breaking change for the scenarios that specify data_home, although it isn't very common in most deployments.

What do you think? @fengjiachun @killme2008, should we break it or not?

@zyy17 zyy17 requested a review from killme2008 May 30, 2025 02:59
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.

2 participants