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: add walconfig dir back #2606

Merged
merged 1 commit into from
Oct 16, 2023
Merged

Conversation

shoothzj
Copy link
Contributor

I hereby agree to the terms of the GreptimeDB CLA

What's changed and what's your intention?

PLEASE DO NOT LEAVE THIS EMPTY !!!

Add Wal dir config back.

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 (optional)

#2312

@killme2008
Copy link
Contributor

Cool! Thank u for your contribution.

Looks like we need to use this parameter in build_log_store:

async fn build_log_store(opts: &DatanodeOptions) -> Result<Arc<RaftEngineLogStore>> {

If it is presented(not none), we use it instead of format!("{}{WAL_DIR}", data_home).

@killme2008
Copy link
Contributor

killme2008 commented Oct 16, 2023

@shoothzj Looks like there is a code format issue.

Please run the cargo fmt and cargo clippy --workspace --all-targets -- -D commands to ensure no code format issues or clippy warnings before committing.

Or you can use a post-commit hook, see the contributing guide:
https://github.com/GreptimeTeam/greptimedb/blob/develop/CONTRIBUTING.md#before-pr

@shoothzj
Copy link
Contributor Author

@killme2008 When I run the cargo clippy --workspace --all-targets -- -D on my macos. It shows

error: failed to run `rustc` to learn about target-specific information

Caused by:
  process didn't exit successfully: `/Users/akka/.rustup/toolchains/nightly-2023-08-07-aarch64-apple-darwin/bin/clippy-driver /Users/akka/.rustup/toolchains/nightly-2023-08-07-aarch64-apple-darwin/bin/rustc - --crate-name ___ --print=file-names '-Wclippy::print_stdout' '-Wclippy::print_stderr' '-Wclippy::implicit_clone' '-Aclippy::needless_pass_by_ref_mut' --crate-type bin --crate-type rlib --crate-type dylib --crate-type cdylib --crate-type staticlib --crate-type proc-macro --print=sysroot --print=split-debuginfo --print=crate-name --print=cfg` (exit status: 1)
  --- stderr
  error: multiple input filenames provided (first two filenames are `-` and `feature="cargo-clippy"`)

@codecov
Copy link

codecov bot commented Oct 16, 2023

Codecov Report

Merging #2606 (dae86a5) into develop (6e87ac0) will decrease coverage by 0.37%.
Report is 1 commits behind head on develop.
The diff coverage is 85.71%.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #2606      +/-   ##
===========================================
- Coverage    85.33%   84.97%   -0.37%     
===========================================
  Files          737      737              
  Lines       118030   118042      +12     
===========================================
- Hits        100724   100306     -418     
- Misses       17306    17736     +430     

@killme2008
Copy link
Contributor

@shoothzj Have you installed clippy? https://github.com/rust-lang/rust-clippy

@killme2008
Copy link
Contributor

Sorry, my mistake. The full command is cargo clippy --workspace --all-targets -- -D warnings.

@killme2008 killme2008 requested a review from waynexia October 16, 2023 09:56
Copy link
Contributor

@killme2008 killme2008 left a comment

Choose a reason for hiding this comment

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

LGTM. Thank u.

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.

Others LGTM

src/common/config/src/lib.rs Show resolved Hide resolved
src/cmd/src/datanode.rs Show resolved Hide resolved
@waynexia waynexia added the docs-required This change requires docs update. label Oct 16, 2023
Signed-off-by: ZhangJian He <[email protected]>
@shoothzj
Copy link
Contributor Author

add docs PR GreptimeTeam/docs#651.
@killme2008 @sunng87 I think this PR can be merged. Thanks :)

@waynexia waynexia enabled auto-merge October 16, 2023 11:16
@waynexia waynexia added this pull request to the merge queue Oct 16, 2023
Merged via the queue into GreptimeTeam:develop with commit d9e7b89 Oct 16, 2023
17 checks passed
@shoothzj shoothzj deleted the add-wal-dir branch October 16, 2023 11:44
paomian pushed a commit to paomian/greptimedb that referenced this pull request Oct 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs-required This change requires docs update.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants