Skip to content

Fix ingested file and direcotry not being sync#5435

Closed
yiwu-arbug wants to merge 10 commits intofacebook:masterfrom
yiwu-arbug:sync_ingest
Closed

Fix ingested file and direcotry not being sync#5435
yiwu-arbug wants to merge 10 commits intofacebook:masterfrom
yiwu-arbug:sync_ingest

Conversation

@yiwu-arbug
Copy link
Copy Markdown
Contributor

@yiwu-arbug yiwu-arbug commented Jun 10, 2019 •
edited by maysamyabandeh
Loading

Summary:
It it not safe to assume application had sync the SST file before ingest it into DB. Also the directory to put the ingested file needs to be fsync, otherwise the file can be lost. For integrity of RocksDB we need to sync the ingested file and directory before apply the change to manifest.

Also syncing after writing global sequence when write_global_seqno=true was removed in #4172. Adding it back.

Fixes #5287.

Test Plan:
Test ingest file with ldb command and observe fsync/fdatasync in strace output. Tried both move_files=true and move_files=false.
https://gist.github.com/yiwu-arbug/650a4023f57979056d83485fa863bef9

More test suggestions are welcome.

Yi Wu added 2 commits June 10, 2019 20:52
sync ingested file
29c8d23 
Signed-off-by: Yi Wu <yiwu@pingcap.com>
update history
8a641b0 
Signed-off-by: Yi Wu <yiwu@pingcap.com>
@yiwu-arbug
Copy link
Copy Markdown
Contributor Author

yiwu-arbug commented Jun 10, 2019

@siying @riversand963 IMO this is a critical fix. PTAL. Thanks.

@ajkr If you have suggestion for test that's welcome. Thanks as well.

riversand963
riversand963 reviewed Jun 10, 2019
View reviewed changes
fix typo
96b52a6 
Signed-off-by: Yi Wu <yiwu@pingcap.com>
yiwu-arbug
yiwu-arbug commented Jun 10, 2019
View reviewed changes
zhangjinpeng87
zhangjinpeng87 reviewed Jun 11, 2019
View reviewed changes
add logging
e5c5e79 
Signed-off-by: Yi Wu <yiwu@pingcap.com>
@yiwu-arbug
Copy link
Copy Markdown
Contributor Author

yiwu-arbug commented Jun 11, 2019

@riversand963 Do you have more comments? thanks!

riversand963
riversand963 reviewed Jun 12, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@riversand963 riversand963 left a comment

Choose a reason for hiding this comment

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

Thanks @yiwu-arbug for the fix. I have taken a pass and left a few comments.

add assert
ae0c400 
Signed-off-by: Yi Wu <yiwu@pingcap.com>
@riversand963
Copy link
Copy Markdown
Contributor

riversand963 commented Jun 12, 2019

The AppVeyor error should have been fixed in master, and I believe it's irrelevant to this PR.
I think the feature LGTM, since it does not weaken the guarantee that RocksDB already provides. Do you plan to add unit test using fault injection env? For example, you can test the behavior of the db when syncing the sst fails.

riversand963
riversand963 approved these changes Jun 12, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@riversand963 riversand963 left a comment

Choose a reason for hiding this comment

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

LGTM

@yiwu-arbug
Copy link
Copy Markdown
Contributor Author

yiwu-arbug commented Jun 12, 2019

@riversand963 sure, I'll add the test.

add test: sync failure should fail ingestion job
94bf1ac 
Signed-off-by: Yi Wu <yiwu@pingcap.com>
@yiwu-arbug
Copy link
Copy Markdown
Contributor Author

yiwu-arbug commented Jun 14, 2019

@riversand963 test added. mind take another look? Thanks.

Yi Wu added 2 commits June 18, 2019 22:13
remove debug log
78dd12e 
Signed-off-by: Yi Wu <yiwu@pingcap.com>
Merge branch 'master' into sync_ingest
d9870b6
@yiwu-arbug
Copy link
Copy Markdown
Contributor Author

yiwu-arbug commented Jun 18, 2019

I didn't notice I left some debug log and failing CI. Fixed. @riversand963 mind take another look? Thanks.

@yiwu-arbug
Copy link
Copy Markdown
Contributor Author

yiwu-arbug commented Jun 19, 2019

@riversand963 kindly ping

@yiwu-arbug yiwu-arbug mentioned this pull request Jun 20, 2019
Merged
@riversand963
Copy link
Copy Markdown
Contributor

riversand963 commented Jun 20, 2019

I think there is compilation error:

In file included from test_util/fault_injection_test_env.cc:14:
./test_util/fault_injection_test_env.h:99:10: error: 'GetRequiredBufferAlignment' overrides a member function but is not marked 'override' [-Werror,-Winconsistent-missing-override]
  size_t GetRequiredBufferAlignment() const {
         ^
./include/rocksdb/env.h:888:18: note: overridden virtual function is here
  virtual size_t GetRequiredBufferAlignment() const { return kDefaultPageSize; }
                 ^
1 error generated.

Yi Wu added 2 commits June 20, 2019 22:38
fix compile error
ff80cc6 
Signed-off-by: Yi Wu <yiwu@pingcap.com>
Merge branch 'master' into sync_ingest
0fc6d54
@yiwu-arbug
Copy link
Copy Markdown
Contributor Author

yiwu-arbug commented Jun 20, 2019

sorry. fixed.

facebook-github-bot
facebook-github-bot reviewed Jun 21, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@riversand963 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@riversand963
Copy link
Copy Markdown
Contributor

riversand963 commented Jun 21, 2019

LGTM

@yiwu-arbug yiwu-arbug deleted the sync_ingest branch June 21, 2019 17:54
@facebook-github-bot
Copy link
Copy Markdown
Contributor

facebook-github-bot commented Jun 21, 2019

@riversand963 merged this pull request in 2730fe6.

merryChris pushed a commit to merryChris/rocksdb that referenced this pull request Nov 18, 2019
@facebook-github-bot
Fix ingested file and direcotry not being sync (facebook#5435)
9ec5987 
Summary:
It it not safe to assume application had sync the SST file before ingest it into DB. Also the directory to put the ingested file needs to be fsync, otherwise the file can be lost. For integrity of RocksDB we need to sync the ingested file and directory before apply the change to manifest.

Also syncing after writing global sequence when write_global_seqno=true was removed in facebook#4172. Adding it back.

Fixes facebook#5287.
Pull Request resolved: facebook#5435

Test Plan:
Test ingest file with ldb command and observe fsync/fdatasync in strace output. Tried both move_files=true and move_files=false.
https://gist.github.com/yiwu-arbug/650a4023f57979056d83485fa863bef9

More test suggestions are welcome.

Differential Revision: D15941675

Pulled By: riversand963

fbshipit-source-id: 389533f3923065a96df2cdde23ff4724a1810d78
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

3 more reviewers

@facebook-github-bot facebook-github-bot facebook-github-bot left review comments

@zhangjinpeng87 zhangjinpeng87 zhangjinpeng87 left review comments

@riversand963 riversand963 riversand963 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

CLA Signed Merged

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

fsync directory after file ingestion

4 participants

@yiwu-arbug @riversand963 @facebook-github-bot @zhangjinpeng87