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.

Sign up for GitHub

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

Jump to bottom

[store] fix: do not fsync sled data in unittets. File::sync_all() takes 10 ~ 30 ms, at worst 500 ms on a Mac #1231

Merged
merged 2 commits into from
Jul 30, 2021
Merged

[store] fix: do not fsync sled data in unittets. File::sync_all() takes 10 ~ 30 ms, at worst 500 ms on a Mac #1231

merged 2 commits into from
Jul 30, 2021

Conversation

drmingdrmer
Copy link
Member

@drmingdrmer drmingdrmer commented Jul 29, 2021 •
edited
Loading

I hereby agree to the terms of the CLA available at: https://datafuse.rs/policies/cla/

Summary

[store] fix: do not fsync sled data in unittets. File::sync_all() takes 10 ~ 30 ms, at worst 500 ms on a Mac

Sled tree wrappers provides an option to not to fsync after write
operation.
This should only be used in testing environment.

For more details about the slow fsync issue:
https://github.com/drmingdrmer/sledtest

Changelog

  • Bug Fix

Related Issues

@drmingdrmer drmingdrmer added this to the v0.5 milestone Jul 29, 2021
@databend-bot databend-bot added the pr-bugfix this PR patches a bug in codebase label Jul 29, 2021
@databend-bot
Copy link
Member

Thanks for the contribution!
I have applied any labels matching special text in your PR Changelog.

Please review the labels and make any necessary changes.

2 similar comments
@databend-bot
Copy link
Member

Thanks for the contribution!
I have applied any labels matching special text in your PR Changelog.

Please review the labels and make any necessary changes.

@databend-bot
Copy link
Member

Thanks for the contribution!
I have applied any labels matching special text in your PR Changelog.

Please review the labels and make any necessary changes.

@drmingdrmer drmingdrmer marked this pull request as ready for review July 29, 2021 16:40
@codecov-commenter
Copy link

codecov-commenter commented Jul 29, 2021 •
edited
Loading

Codecov Report

Merging #1231 (f5bcfc9) into master (5e8b747) will increase coverage by 0%.
The diff coverage is 98%.

Impacted file tree graph

@@          Coverage Diff           @@
##           master   #1231   +/-   ##
======================================
  Coverage      72%     72%           
======================================
  Files         464     464           
  Lines       27465   27499   +34     
======================================
+ Hits        19865   19901   +36     
+ Misses       7600    7598    -2
Impacted Files Coverage Δ
...estore/store/src/meta_service/sled_vartype_tree.rs 93% <95%> (+<1%) ⬆️
fusestore/store/src/configs/config.rs 97% <100%> (+<1%) ⬆️
fusestore/store/src/meta_service/raft_log.rs 93% <100%> (+<1%) ⬆️
fusestore/store/src/meta_service/raft_log_test.rs 96% <100%> (ø)
fusestore/store/src/meta_service/raftmeta.rs 77% <100%> (ø)
fusestore/store/src/meta_service/sled_tree.rs 93% <100%> (+<1%) ⬆️
fusestore/store/src/meta_service/sled_tree_test.rs 96% <100%> (ø)
...e/store/src/meta_service/sled_vartype_tree_test.rs 97% <100%> (ø)
...pelines/transforms/transform_aggregator_partial.rs 87% <0%> (-2%) ⬇️
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5e8b747...f5bcfc9. Read the comment docs.

BohuTANG
BohuTANG reviewed Jul 30, 2021
View reviewed changes
fusestore/store/src/tests/service.rs
@@ -51,6 +51,9 @@ pub struct StoreTestContext {
pub fn new_test_context() -> StoreTestContext {
let mut config = configs::Config::empty();

// On mac File::sync_all() takes 10 ms ~ 30 ms, 500 ms at worst, which very likely to fail a test.
Copy link
Member

Choose a reason for hiding this comment

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

Can we check the OS if it's MACOS and set it?

Copy link
Member Author

Choose a reason for hiding this comment

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

Can we check the OS if it's MACOS and set it?

Good idea 🤔

Copy link

@templexxx templexxx Jul 31, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

All Non-Enterprise device which has no Enhanced Power Loss Data Protection cannot sync enough fast. I think it's not just MacOS issue

Copy link
Member Author

Choose a reason for hiding this comment

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

@templexxx
You are totally right, as always! :DDD
Any suggestions for building a stable durable storage system would be appreciated!

@drmingdrmer
[store] fix: do not fsync sled data in unittets. File::sync_all() tak…
90ae18c 
…es 10 ~ 30 ms, at worst 500 ms on a Mac

Sled tree wrappers provides an option to not to fsync after write
operation.
This should only be used in testing environment.

For more details about the slow fsync issue:
https://github.com/drmingdrmer/sledtest
@drmingdrmer
[store]: refactor: only disable meta fsync if OS==mac
f5bcfc9
BohuTANG
BohuTANG approved these changes Jul 30, 2021
View reviewed changes
Copy link
Member

@BohuTANG BohuTANG left a comment

Choose a reason for hiding this comment

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

LGTM

@databend-bot
Copy link
Member

CI Passed
Reviewer Approved
Let's Merge

@databend-bot databend-bot merged commit 6adf22e into databendlabs:master Jul 30, 2021
@drmingdrmer drmingdrmer deleted the no-sync branch July 30, 2021 02:44
This was referenced Jul 31, 2021
Closed
Closed
@mergify
Copy link
Contributor

mergify bot commented Mar 14, 2022

Thanks for the contribution!
I have applied any labels matching special text in your PR Changelog.

Please review the labels and make any necessary changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@templexxx templexxx templexxx left review comments

@BohuTANG BohuTANG BohuTANG approved these changes

@ariesdevil ariesdevil Awaiting requested review from ariesdevil

@dantengsky dantengsky Awaiting requested review from dantengsky

Assignees
No one assigned
Labels
pr-bugfix this PR patches a bug in codebase
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@drmingdrmer @databend-bot @codecov-commenter @BohuTANG @templexxx