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: expose DatanodeBuilder::build_object_store_manager() and MitoConfig::sanitize() #4212

Merged

Conversation

zyy17
Copy link
Collaborator

@zyy17 zyy17 commented Jun 25, 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?

Some minor refactoring:

  1. Expose DatanodeBuilder::build_object_store_manager()

    The scenario is the following code:

    ...
    let object_storage_manager =
             datanode::datanode::DatanodeBuilder::build_object_store_manager(&storage_config)
                 .await
                 .context(BuildObjectStoreManagerSnafu)?;
    ...
    let compaction_region = open_compaction_region(
                     &open_region_request,
                     &self.mito_config,
                     object_storage_manager.clone(),
                 )
    ...
  2. Expose MitoConfig::sanitize() to initialize Mito config outside the crate;

2. Set default value for intermediate_path instead of empty value(it will failed to create a access layer if using open_compaction_region() with default Mito config) (cc @zhongzc);

Checklist

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

Summary by CodeRabbit

  • New Features

    • Set a default data home directory for file storage, enhancing ease of configuration.
    • Updated intermediate path configurations to use the default data home directory across various components.
  • Documentation

    • Revised configuration documentation to reflect changes in default paths.
  • Configuration

    • Updated example configuration files to include the new default paths for intermediate storage.

@zyy17 zyy17 requested review from evenyag, v0y4g3r, waynexia and a team as code owners June 25, 2024 11:49
Copy link
Contributor

coderabbitai bot commented Jun 25, 2024

Warning

Rate limit exceeded

@zyy17 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 45 minutes and 4 seconds before requesting another review.

How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

Commits

Files that changed from the base of the PR and between 2256eb1 and ff46a5a.

Walkthrough

The recent updates introduce a new constant, DEFAULT_DATA_HOME, set to "/tmp/greptimedb", standardizing the default storage path across multiple configuration files and modules. Key configurations and default paths within DataNode options, InvertedIndexConfig, and various example configuration files have been modified to use this constant, promoting consistency and centralized configuration management.

Changes

Files/Paths Change Summaries
src/common/config/src/lib.rs Introduced and exported new constant DEFAULT_DATA_HOME as "/tmp/greptimedb".
src/datanode/src/config.rs Updated StorageConfig to use common_config::DEFAULT_DATA_HOME and modified functions to align with this update.
src/.../datanode.rs Adjusted imports to include StorageConfig.
src/mito2/src/config.rs Changed intermediate_path in InvertedIndexConfig's default implementation to use join_dir(common_config::DEFAULT_DATA_HOME, "index_intermediate").
config/config.md Updated region_engine.mito.inverted_index.intermediate_path to /tmp/greptimedb/index_intermediate/.
config/datanode.example.toml Adjusted intermediate_path setting to /tmp/greptimedb/index_intermediate/.
config/standalone.example.toml Modified intermediate_path setting to /tmp/greptimedb/index_intermediate/.
tests-integration/tests/http.rs Updated intermediate_path configuration to /tmp/greptimedb/index_intermediate/.

Oh coding paths, now aligned,
In /tmp/greptimedb they bind,
With constants in place,
Storage paths embrace,
Order in configs defined.


Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

Share
Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai generate interesting stats about this repository and render them as a table.
    • @coderabbitai show all the console.log statements in this repository.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (invoked as PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@zyy17 zyy17 requested a review from zhongzc June 25, 2024 11:49
@github-actions github-actions bot added the docs-not-required This change does not impact docs. label Jun 25, 2024
@zyy17 zyy17 removed the request for review from a team June 25, 2024 11:49
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between f5ac158 and 59462b5.

Files selected for processing (4)
  • src/common/config/src/lib.rs (1 hunks)
  • src/datanode/src/config.rs (2 hunks)
  • src/datanode/src/datanode.rs (4 hunks)
  • src/mito2/src/config.rs (1 hunks)
Files skipped from review due to trivial changes (1)
  • src/common/config/src/lib.rs
Additional comments not posted (3)
src/datanode/src/config.rs (1)

72-72: Standardized default data directory.

The change to use common_config::DEFAULT_DATA_HOME for the data_home field in StorageConfig is a positive step towards consistency across configurations. This ensures that all components of the system default to the same directory unless explicitly overridden.

src/mito2/src/config.rs (1)

301-301: Set robust default for intermediate index storage path.

Setting the intermediate_path in InvertedIndexConfig to a default value using join_dir function ensures that there is always a valid directory specified. This change prevents potential runtime errors and simplifies configuration by providing sensible defaults.

src/datanode/src/datanode.rs (1)

273-285: Refactored and exposed build_object_store_manager.

The refactoring to make build_object_store_manager an asynchronous function that is publicly accessible is a crucial improvement. It allows for more flexible and dynamic construction of ObjectStoreManager instances based on runtime configuration. This change aligns with modern asynchronous programming practices in Rust and enhances modularity.

@zyy17 zyy17 force-pushed the refactor/expose-api-for-object-store-manager branch from 59462b5 to b51d8bf Compare June 26, 2024 04:06
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 59462b5 and b51d8bf.

Files selected for processing (6)
  • config/config.md (2 hunks)
  • config/datanode.example.toml (1 hunks)
  • config/standalone.example.toml (1 hunks)
  • src/common/config/src/lib.rs (1 hunks)
  • src/datanode/src/config.rs (2 hunks)
  • src/mito2/src/config.rs (1 hunks)
Files skipped from review due to trivial changes (1)
  • config/standalone.example.toml
Files skipped from review as they are similar to previous changes (3)
  • src/common/config/src/lib.rs
  • src/datanode/src/config.rs
  • src/mito2/src/config.rs
Additional context used
LanguageTool
config/config.md

[uncategorized] ~31-~31: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’). (AUTO_HYPHEN)
Context: ...for Certificate and key file change and auto reload.
For now, gRPC tls config does not ...


[uncategorized] ~31-~31: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’). (AUTO_HYPHEN)
Context: ...r now, gRPC tls config does not support auto reload. | | mysql | -- | -- | MySQL server o...


[uncategorized] ~40-~40: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’). (AUTO_HYPHEN)
Context: ...for Certificate and key file change and auto reload | | postgres | -- | -- | PostgresSQL ...


[uncategorized] ~49-~49: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’). (AUTO_HYPHEN)
Context: ...for Certificate and key file change and auto reload | | opentsdb | -- | -- | OpenTSDB pro...


[grammar] ~93-~93: “Google” is a proper noun and needs to be capitalized. (A_GOOGLE)
Context: ...e| String |None` | The scope of the google cloud storage.
**It's only used whe...


[grammar] ~94-~94: “Google” is a proper noun and needs to be capitalized. (A_GOOGLE)
Context: ...g | None | The credential path of the google cloud storage.
**It's only used whe...


[uncategorized] ~108-~108: Possible missing comma found. (AI_HYDRA_LEO_MISSING_COMMA)
Context: .... If not set, it's default to 1/8 of OS memory with a max limitation of 1GB. | | `regi...


[style] ~134-~134: To form a complete sentence, be sure to include a subject. (MISSING_IT_THERE)
Context: ...vel| String |None| The log level. Can beinfo/debug/warn/error. | | ...


[grammar] ~143-~143: Consider using either the past participle “recommended” or the present participle “recommending” here. (BEEN_PART_AGREEMENT)
Context: ...For standalone mode, self_import is recommend to collect metrics generated by itself ...


[uncategorized] ~178-~178: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’). (AUTO_HYPHEN)
Context: ...for Certificate and key file change and auto reload.
For now, gRPC tls config does not ...


[uncategorized] ~178-~178: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’). (AUTO_HYPHEN)
Context: ...r now, gRPC tls config does not support auto reload. | | mysql | -- | -- | MySQL server o...


[uncategorized] ~187-~187: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’). (AUTO_HYPHEN)
Context: ...for Certificate and key file change and auto reload | | postgres | -- | -- | PostgresSQL ...


[uncategorized] ~196-~196: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’). (AUTO_HYPHEN)
Context: ...for Certificate and key file change and auto reload | | opentsdb | -- | -- | OpenTSDB pro...


[style] ~220-~220: To form a complete sentence, be sure to include a subject. (MISSING_IT_THERE)
Context: ...vel| String |None| The log level. Can beinfo/debug/warn/error. | | ...


[grammar] ~229-~229: Consider using either the past participle “recommended” or the present participle “recommending” here. (BEEN_PART_AGREEMENT)
Context: ...For standalone mode, self_import is recommend to collect metrics generated by itself ...


[style] ~282-~282: To form a complete sentence, be sure to include a subject. (MISSING_IT_THERE)
Context: ...vel| String |None| The log level. Can beinfo/debug/warn/error. | | ...


[grammar] ~291-~291: Consider using either the past participle “recommended” or the present participle “recommending” here. (BEEN_PART_AGREEMENT)
Context: ...For standalone mode, self_import is recommend to collect metrics generated by itself ...


[uncategorized] ~325-~325: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’). (AUTO_HYPHEN)
Context: ...for Certificate and key file change and auto reload.
For now, gRPC tls config does not ...


[uncategorized] ~325-~325: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’). (AUTO_HYPHEN)
Context: ...r now, gRPC tls config does not support auto reload. | | runtime | -- | -- | The runtime ...


[grammar] ~373-~373: “Google” is a proper noun and needs to be capitalized. (A_GOOGLE)
Context: ...e| String |None` | The scope of the google cloud storage.
**It's only used whe...


[grammar] ~374-~374: “Google” is a proper noun and needs to be capitalized. (A_GOOGLE)
Context: ...g | None | The credential path of the google cloud storage.
**It's only used whe...


[uncategorized] ~388-~388: Possible missing comma found. (AI_HYDRA_LEO_MISSING_COMMA)
Context: .... If not set, it's default to 1/8 of OS memory with a max limitation of 1GB. | | `regi...


[style] ~414-~414: To form a complete sentence, be sure to include a subject. (MISSING_IT_THERE)
Context: ...vel| String |None| The log level. Can beinfo/debug/warn/error. | | ...


[grammar] ~423-~423: Consider using either the past participle “recommended” or the present participle “recommending” here. (BEEN_PART_AGREEMENT)
Context: ...For standalone mode, self_import is recommend to collect metrics generated by itself ...

Markdownlint
config/config.md

5-5: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


6-6: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


7-7: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


151-151: Expected: 1; Actual: 2 (MD012, no-multiple-blanks)
Multiple consecutive blank lines


237-237: Expected: 1; Actual: 2 (MD012, no-multiple-blanks)
Multiple consecutive blank lines


299-299: Expected: 1; Actual: 2 (MD012, no-multiple-blanks)
Multiple consecutive blank lines


37-37: null (MD034, no-bare-urls)
Bare URL used


184-184: null (MD034, no-bare-urls)
Bare URL used


246-246: null (MD034, no-bare-urls)
Bare URL used

Additional comments not posted (2)
config/datanode.example.toml (1)

419-419: Proper setting of default intermediate_path

The update to set a default value for intermediate_path is a positive change. It ensures that the system has a valid directory to operate even if the user does not specify one. This prevents potential runtime errors and aligns with the goal of robustness in system configuration.

config/config.md (1)

126-126: Updated default value for intermediate_path is well-documented.

The change in the default value for intermediate_path from an empty string to /tmp/greptimedb/index_intermediate/ is correctly documented here. This aligns with the PR's objective to prevent failures when open_compaction_region() is used with the default configuration.

config/config.md Outdated Show resolved Hide resolved
@zyy17 zyy17 force-pushed the refactor/expose-api-for-object-store-manager branch from b51d8bf to 826befa Compare June 26, 2024 05:18
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

Outside diff range and nitpick comments (1)
config/config.md (1)

Line range hint 151-151: Remove extra blank lines to adhere to Markdown standards.

There are multiple consecutive blank lines that do not adhere to Markdown best practices and can be cleaned up for better readability.

- 

Also applies to: 237-237, 299-299

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between b51d8bf and 826befa.

Files selected for processing (7)
  • config/config.md (2 hunks)
  • config/datanode.example.toml (1 hunks)
  • config/standalone.example.toml (1 hunks)
  • src/common/config/src/lib.rs (1 hunks)
  • src/datanode/src/config.rs (2 hunks)
  • src/mito2/src/config.rs (1 hunks)
  • tests-integration/tests/http.rs (1 hunks)
Files skipped from review due to trivial changes (1)
  • tests-integration/tests/http.rs
Files skipped from review as they are similar to previous changes (5)
  • config/datanode.example.toml
  • config/standalone.example.toml
  • src/common/config/src/lib.rs
  • src/datanode/src/config.rs
  • src/mito2/src/config.rs
Additional context used
LanguageTool
config/config.md

[uncategorized] ~31-~31: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’). (AUTO_HYPHEN)
Context: ...for Certificate and key file change and auto reload.
For now, gRPC tls config does not ...


[uncategorized] ~31-~31: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’). (AUTO_HYPHEN)
Context: ...r now, gRPC tls config does not support auto reload. | | mysql | -- | -- | MySQL server o...


[uncategorized] ~40-~40: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’). (AUTO_HYPHEN)
Context: ...for Certificate and key file change and auto reload | | postgres | -- | -- | PostgresSQL ...


[uncategorized] ~49-~49: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’). (AUTO_HYPHEN)
Context: ...for Certificate and key file change and auto reload | | opentsdb | -- | -- | OpenTSDB pro...


[grammar] ~93-~93: “Google” is a proper noun and needs to be capitalized. (A_GOOGLE)
Context: ...e| String |None` | The scope of the google cloud storage.
**It's only used whe...


[grammar] ~94-~94: “Google” is a proper noun and needs to be capitalized. (A_GOOGLE)
Context: ...g | None | The credential path of the google cloud storage.
**It's only used whe...


[style] ~134-~134: To form a complete sentence, be sure to include a subject. (MISSING_IT_THERE)
Context: ...vel| String |None| The log level. Can beinfo/debug/warn/error. | | ...


[grammar] ~143-~143: Consider using either the past participle “recommended” or the present participle “recommending” here. (BEEN_PART_AGREEMENT)
Context: ...For standalone mode, self_import is recommend to collect metrics generated by itself ...


[uncategorized] ~178-~178: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’). (AUTO_HYPHEN)
Context: ...for Certificate and key file change and auto reload.
For now, gRPC tls config does not ...


[uncategorized] ~178-~178: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’). (AUTO_HYPHEN)
Context: ...r now, gRPC tls config does not support auto reload. | | mysql | -- | -- | MySQL server o...


[uncategorized] ~187-~187: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’). (AUTO_HYPHEN)
Context: ...for Certificate and key file change and auto reload | | postgres | -- | -- | PostgresSQL ...


[uncategorized] ~196-~196: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’). (AUTO_HYPHEN)
Context: ...for Certificate and key file change and auto reload | | opentsdb | -- | -- | OpenTSDB pro...


[style] ~220-~220: To form a complete sentence, be sure to include a subject. (MISSING_IT_THERE)
Context: ...vel| String |None| The log level. Can beinfo/debug/warn/error. | | ...


[grammar] ~229-~229: Consider using either the past participle “recommended” or the present participle “recommending” here. (BEEN_PART_AGREEMENT)
Context: ...For standalone mode, self_import is recommend to collect metrics generated by itself ...


[style] ~282-~282: To form a complete sentence, be sure to include a subject. (MISSING_IT_THERE)
Context: ...vel| String |None| The log level. Can beinfo/debug/warn/error. | | ...


[grammar] ~291-~291: Consider using either the past participle “recommended” or the present participle “recommending” here. (BEEN_PART_AGREEMENT)
Context: ...For standalone mode, self_import is recommend to collect metrics generated by itself ...


[uncategorized] ~325-~325: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’). (AUTO_HYPHEN)
Context: ...for Certificate and key file change and auto reload.
For now, gRPC tls config does not ...


[uncategorized] ~325-~325: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’). (AUTO_HYPHEN)
Context: ...r now, gRPC tls config does not support auto reload. | | runtime | -- | -- | The runtime ...


[grammar] ~373-~373: “Google” is a proper noun and needs to be capitalized. (A_GOOGLE)
Context: ...e| String |None` | The scope of the google cloud storage.
**It's only used whe...


[grammar] ~374-~374: “Google” is a proper noun and needs to be capitalized. (A_GOOGLE)
Context: ...g | None | The credential path of the google cloud storage.
**It's only used whe...


[style] ~414-~414: To form a complete sentence, be sure to include a subject. (MISSING_IT_THERE)
Context: ...vel| String |None| The log level. Can beinfo/debug/warn/error. | | ...


[grammar] ~423-~423: Consider using either the past participle “recommended” or the present participle “recommending” here. (BEEN_PART_AGREEMENT)
Context: ...For standalone mode, self_import is recommend to collect metrics generated by itself ...

Markdownlint
config/config.md

5-5: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


6-6: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


7-7: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


151-151: Expected: 1; Actual: 2 (MD012, no-multiple-blanks)
Multiple consecutive blank lines


237-237: Expected: 1; Actual: 2 (MD012, no-multiple-blanks)
Multiple consecutive blank lines


299-299: Expected: 1; Actual: 2 (MD012, no-multiple-blanks)
Multiple consecutive blank lines


37-37: null (MD034, no-bare-urls)
Bare URL used


184-184: null (MD034, no-bare-urls)
Bare URL used


246-246: null (MD034, no-bare-urls)
Bare URL used

Additional comments not posted (1)
config/config.md (1)

126-126: Enhance clarity in the description of the default value for intermediate_path.

The documentation correctly reflects the new default value for intermediate_path. However, to enhance clarity, consider specifying that this path is used for storing intermediate files during external sorting operations, as this detail enhances user understanding of the configuration's purpose.

- File system path to store intermediate files for external sorting (default `{data_home}/index_intermediate`).
+ File system path to store intermediate files during external sorting operations. Default is `{data_home}/index_intermediate`.

@zyy17 zyy17 force-pushed the refactor/expose-api-for-object-store-manager branch from 826befa to 2256eb1 Compare June 26, 2024 05:34
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 826befa and 2256eb1.

Files selected for processing (7)
  • config/config.md (2 hunks)
  • config/datanode.example.toml (1 hunks)
  • config/standalone.example.toml (1 hunks)
  • src/common/config/src/lib.rs (1 hunks)
  • src/datanode/src/config.rs (2 hunks)
  • src/mito2/src/config.rs (1 hunks)
  • tests-integration/tests/http.rs (1 hunks)
Files skipped from review as they are similar to previous changes (6)
  • config/datanode.example.toml
  • config/standalone.example.toml
  • src/common/config/src/lib.rs
  • src/datanode/src/config.rs
  • src/mito2/src/config.rs
  • tests-integration/tests/http.rs
Additional context used
LanguageTool
config/config.md

[uncategorized] ~31-~31: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’). (AUTO_HYPHEN)
Context: ...for Certificate and key file change and auto reload.
For now, gRPC tls config does not ...


[uncategorized] ~31-~31: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’). (AUTO_HYPHEN)
Context: ...r now, gRPC tls config does not support auto reload. | | mysql | -- | -- | MySQL server o...


[uncategorized] ~40-~40: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’). (AUTO_HYPHEN)
Context: ...for Certificate and key file change and auto reload | | postgres | -- | -- | PostgresSQL ...


[uncategorized] ~49-~49: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’). (AUTO_HYPHEN)
Context: ...for Certificate and key file change and auto reload | | opentsdb | -- | -- | OpenTSDB pro...


[grammar] ~93-~93: “Google” is a proper noun and needs to be capitalized. (A_GOOGLE)
Context: ...e| String |None` | The scope of the google cloud storage.
**It's only used whe...


[grammar] ~94-~94: “Google” is a proper noun and needs to be capitalized. (A_GOOGLE)
Context: ...g | None | The credential path of the google cloud storage.
**It's only used whe...


[style] ~134-~134: To form a complete sentence, be sure to include a subject. (MISSING_IT_THERE)
Context: ...vel| String |None| The log level. Can beinfo/debug/warn/error. | | ...


[grammar] ~143-~143: Consider using either the past participle “recommended” or the present participle “recommending” here. (BEEN_PART_AGREEMENT)
Context: ...For standalone mode, self_import is recommend to collect metrics generated by itself ...


[uncategorized] ~178-~178: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’). (AUTO_HYPHEN)
Context: ...for Certificate and key file change and auto reload.
For now, gRPC tls config does not ...


[uncategorized] ~178-~178: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’). (AUTO_HYPHEN)
Context: ...r now, gRPC tls config does not support auto reload. | | mysql | -- | -- | MySQL server o...


[uncategorized] ~187-~187: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’). (AUTO_HYPHEN)
Context: ...for Certificate and key file change and auto reload | | postgres | -- | -- | PostgresSQL ...


[uncategorized] ~196-~196: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’). (AUTO_HYPHEN)
Context: ...for Certificate and key file change and auto reload | | opentsdb | -- | -- | OpenTSDB pro...


[style] ~220-~220: To form a complete sentence, be sure to include a subject. (MISSING_IT_THERE)
Context: ...vel| String |None| The log level. Can beinfo/debug/warn/error. | | ...


[grammar] ~229-~229: Consider using either the past participle “recommended” or the present participle “recommending” here. (BEEN_PART_AGREEMENT)
Context: ...For standalone mode, self_import is recommend to collect metrics generated by itself ...


[style] ~282-~282: To form a complete sentence, be sure to include a subject. (MISSING_IT_THERE)
Context: ...vel| String |None| The log level. Can beinfo/debug/warn/error. | | ...


[grammar] ~291-~291: Consider using either the past participle “recommended” or the present participle “recommending” here. (BEEN_PART_AGREEMENT)
Context: ...For standalone mode, self_import is recommend to collect metrics generated by itself ...


[uncategorized] ~325-~325: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’). (AUTO_HYPHEN)
Context: ...for Certificate and key file change and auto reload.
For now, gRPC tls config does not ...


[uncategorized] ~325-~325: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’). (AUTO_HYPHEN)
Context: ...r now, gRPC tls config does not support auto reload. | | runtime | -- | -- | The runtime ...


[grammar] ~373-~373: “Google” is a proper noun and needs to be capitalized. (A_GOOGLE)
Context: ...e| String |None` | The scope of the google cloud storage.
**It's only used whe...


[grammar] ~374-~374: “Google” is a proper noun and needs to be capitalized. (A_GOOGLE)
Context: ...g | None | The credential path of the google cloud storage.
**It's only used whe...


[style] ~414-~414: To form a complete sentence, be sure to include a subject. (MISSING_IT_THERE)
Context: ...vel| String |None| The log level. Can beinfo/debug/warn/error. | | ...


[grammar] ~423-~423: Consider using either the past participle “recommended” or the present participle “recommending” here. (BEEN_PART_AGREEMENT)
Context: ...For standalone mode, self_import is recommend to collect metrics generated by itself ...

Markdownlint
config/config.md

5-5: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


6-6: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


7-7: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


151-151: Expected: 1; Actual: 2 (MD012, no-multiple-blanks)
Multiple consecutive blank lines


237-237: Expected: 1; Actual: 2 (MD012, no-multiple-blanks)
Multiple consecutive blank lines


299-299: Expected: 1; Actual: 2 (MD012, no-multiple-blanks)
Multiple consecutive blank lines


37-37: null (MD034, no-bare-urls)
Bare URL used


184-184: null (MD034, no-bare-urls)
Bare URL used


246-246: null (MD034, no-bare-urls)
Bare URL used

Additional comments not posted (1)
config/config.md (1)

126-126: Default Path for Intermediate Files Correctly Set

The new default path for intermediate_path is consistently set across different configurations to /tmp/greptimedb/index_intermediate/. This change ensures that there are no failures due to an empty path value and standardizes file storage paths across the system.

Also applies to: 406-406

Copy link

codecov bot commented Jun 26, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.58%. Comparing base (ef935a1) to head (ff46a5a).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4212      +/-   ##
==========================================
- Coverage   84.86%   84.58%   -0.29%     
==========================================
  Files        1045     1045              
  Lines      184251   184250       -1     
==========================================
- Hits       156373   155850     -523     
- Misses      27878    28400     +522     

src/mito2/src/config.rs Outdated Show resolved Hide resolved
@zyy17 zyy17 requested a review from evenyag June 27, 2024 15:24
@zyy17 zyy17 force-pushed the refactor/expose-api-for-object-store-manager branch from 2256eb1 to f2213b8 Compare June 28, 2024 12:06
@zyy17 zyy17 changed the title refactor: expose build_object_store_manager() and fix empty intermediate_path error refactor: expose DatanodeBuilder::build_object_store_manager() and MitoConfig::sanitize() Jun 28, 2024
@zyy17 zyy17 force-pushed the refactor/expose-api-for-object-store-manager branch from f2213b8 to ff46a5a Compare June 28, 2024 12:13
@zyy17
Copy link
Collaborator Author

zyy17 commented Jul 1, 2024

@evenyag @zhongzc PTAL

Copy link
Member

@WenyXu WenyXu left a comment

Choose a reason for hiding this comment

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

LGTM

@zyy17 zyy17 added this pull request to the merge queue Jul 1, 2024
Merged via the queue into GreptimeTeam:main with commit fe2c5c3 Jul 1, 2024
50 checks passed
@zyy17 zyy17 deleted the refactor/expose-api-for-object-store-manager branch July 1, 2024 06:52
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.

4 participants