Fix OnFlushCompleted fired before flush result write to MANIFEST #5908

yiwu-arbug · 2019-10-11T22:05:12Z

Summary:
When there are concurrent flush job on the same CF, OnFlushCompleted can be called before the flush result being install to LSM. Fixing the issue by passing FlushJobInfo through MemTable, and the thread who commit the flush result can fetch the FlushJobInfo and fire OnFlushCompleted on behave of the thread actually writing the SST.

Fix #5892

Test Plan:
Add new test. The test will fail without the fix.

Signed-off-by: Yi Wu <yiwu@pingcap.com>

db/flush_job.h

Signed-off-by: Yi Wu <yiwu@pingcap.com>

riversand963

Thanks @yiwu-arbug for the fix. I have taken a pass and left a few minor comments.
The appveyor is still failing.

db/db_flush_test.cc

db/db_impl/db_impl.h

facebook-github-bot

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

Signed-off-by: Yi Wu <yiwu@pingcap.com>

facebook-github-bot · 2019-10-14T22:01:48Z

@yiwu-arbug has updated the pull request. Re-import the pull request

facebook-github-bot · 2019-10-14T22:11:38Z

@yiwu-arbug has updated the pull request. Re-import the pull request

Signed-off-by: Yi Wu <yiwu@pingcap.com>

facebook-github-bot · 2019-10-14T22:42:27Z

@yiwu-arbug has updated the pull request. Re-import the pull request

riversand963

LGTM. Let's wait for CI tests results.

facebook-github-bot

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

yiwu-arbug · 2019-10-15T05:16:41Z

What's the linter error? Thanks.

riversand963 · 2019-10-15T05:38:58Z

There are several of them.

db_flush_test.cc

line 365, seq1 uninitialized.
line 366, seq2 uninitizlized.
line 371, first_flush_pending unused.

db_impl_compaction_flush.cc

line 386, Getting a pointer to the first element of a vector can be UB If a vector is empty, getting a pointer to the start of its data block through &v[0] is undefined behaviour. If you want a pointer to the vector's memory, please call .data()
line 508, Array accesses with [] or calls to front() or back() will segfault when called on empty arrays. Please make sure that you use them in a safe context by either explicitly checking the arrays size before you access it or by asserting that it is not empty. Local variable all_mutable_cf_options accessed at line 508 may be empty.

Signed-off-by: Yi Wu <yiwu@pingcap.com>

facebook-github-bot · 2019-10-15T06:20:01Z

@yiwu-arbug has updated the pull request. Re-import the pull request

facebook-github-bot

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

riversand963 · 2019-10-15T17:03:50Z

Still lint error. I can push to your branch if you want. @yiwu-arbug

yiwu-arbug · 2019-10-15T18:28:11Z

Still lint error. I can push to your branch if you want. @yiwu-arbug

go ahead. I think you can edit the PR from github UI.

Assert `file_meta` is non-empty before accessing it via subscription.

facebook-github-bot · 2019-10-15T19:53:01Z

@yiwu-arbug has updated the pull request. Re-import the pull request

facebook-github-bot

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

facebook-github-bot · 2019-10-16T16:01:09Z

@yiwu-arbug has updated the pull request. Re-import the pull request

facebook-github-bot

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

riversand963 · 2019-10-16T17:23:58Z

Double checked the lint error and suggested change shown as below. I think we can just ignore it given that we assert the non-emptiness of file_meta.

-        jobs[0]->Run(&logs_with_prep_tracker_, &file_meta[0]);
+        jobs[0]->Run(&logs_with_prep_tracker_, file_meta.data());

yiwu-arbug · 2019-10-16T17:45:49Z

@riversand963 thanks much for merging!

facebook-github-bot · 2019-10-16T19:09:10Z

@riversand963 merged this pull request in 1f9d7c0.

…ebook#5908) (#127) Summary: When there are concurrent flush job on the same CF, `OnFlushCompleted` can be called before the flush result being install to LSM. Fixing the issue by passing `FlushJobInfo` through `MemTable`, and the thread who commit the flush result can fetch the `FlushJobInfo` and fire `OnFlushCompleted` on behave of the thread actually writing the SST. Fix facebook#5892 Pull Request resolved: facebook#5908 Test Plan: Add new test. The test will fail without the fix. Differential Revision: D17916144 Pulled By: riversand963 fbshipit-source-id: e18df67d9533b5baee52ae3605026cdeb05cbe10

…ebook#5908) Summary: When there are concurrent flush job on the same CF, `OnFlushCompleted` can be called before the flush result being install to LSM. Fixing the issue by passing `FlushJobInfo` through `MemTable`, and the thread who commit the flush result can fetch the `FlushJobInfo` and fire `OnFlushCompleted` on behave of the thread actually writing the SST. Fix facebook#5892 Pull Request resolved: facebook#5908 Test Plan: Add new test. The test will fail without the fix. Differential Revision: D17916144 Pulled By: riversand963 fbshipit-source-id: e18df67d9533b5baee52ae3605026cdeb05cbe10

…ebook#5908) (#127) (#130) Summary: When there are concurrent flush job on the same CF, `OnFlushCompleted` can be called before the flush result being install to LSM. Fixing the issue by passing `FlushJobInfo` through `MemTable`, and the thread who commit the flush result can fetch the `FlushJobInfo` and fire `OnFlushCompleted` on behave of the thread actually writing the SST. Fix facebook#5892 Pull Request resolved: facebook#5908 Test Plan: Add new test. The test will fail without the fix. Differential Revision: D17916144 Pulled By: riversand963 fbshipit-source-id: e18df67d9533b5baee52ae3605026cdeb05cbe10 Signed-off-by: Yi Wu <yiwu@pingcap.com>

Summary: PR #5908 added `flush_jobs_info_` to `FlushJob` to make sure `OnFlushCompleted()` is called after committing flush results to MANIFEST. However, `flush_jobs_info_` is not updated in atomic flush, causing `NotifyOnFlushCompleted()` to skip `OnFlushCompleted()`. This PR fixes this, in a similar way to #5908 that handles regular flush. Pull Request resolved: #8585 Test Plan: make check Reviewed By: jay-zhuang Differential Revision: D29913720 Pulled By: riversand963 fbshipit-source-id: 4ff023c98372fa2c93188d4a5c8a4e9ffa0f4dda

Summary: PR facebook#5908 added `flush_jobs_info_` to `FlushJob` to make sure `OnFlushCompleted()` is called after committing flush results to MANIFEST. However, `flush_jobs_info_` is not updated in atomic flush, causing `NotifyOnFlushCompleted()` to skip `OnFlushCompleted()`. This PR fixes this, in a similar way to facebook#5908 that handles regular flush. Pull Request resolved: facebook#8585 Test Plan: make check Reviewed By: jay-zhuang Differential Revision: D29913720 Pulled By: riversand963 fbshipit-source-id: 4ff023c98372fa2c93188d4a5c8a4e9ffa0f4dda

Summary: PR facebook/rocksdb#5908 added `flush_jobs_info_` to `FlushJob` to make sure `OnFlushCompleted()` is called after committing flush results to MANIFEST. However, `flush_jobs_info_` is not updated in atomic flush, causing `NotifyOnFlushCompleted()` to skip `OnFlushCompleted()`. This PR fixes this, in a similar way to facebook/rocksdb#5908 that handles regular flush. Pull Request resolved: facebook/rocksdb#8585 Test Plan: make check Reviewed By: jay-zhuang Differential Revision: D29913720 Pulled By: riversand963 fbshipit-source-id: 4ff023c98372fa2c93188d4a5c8a4e9ffa0f4dda

Summary: PR facebook/rocksdb#5908 added `flush_jobs_info_` to `FlushJob` to make sure `OnFlushCompleted()` is called after committing flush results to MANIFEST. However, `flush_jobs_info_` is not updated in atomic flush, causing `NotifyOnFlushCompleted()` to skip `OnFlushCompleted()`. This PR fixes this, in a similar way to facebook/rocksdb#5908 that handles regular flush. Pull Request resolved: facebook/rocksdb#8585 Test Plan: make check Reviewed By: jay-zhuang Differential Revision: D29913720 Pulled By: riversand963 fbshipit-source-id: 4ff023c98372fa2c93188d4a5c8a4e9ffa0f4dda Co-authored-by: Yanqin Jin <yanqin@fb.com>

Yi Wu added 4 commits October 11, 2019 05:26

fire OnFlushCompleted after flush result committed

2426d4c

Signed-off-by: Yi Wu <yiwu@pingcap.com>

fix memtable_list_test

022fc22

Signed-off-by: Yi Wu <yiwu@pingcap.com>

add test

b9401ec

Signed-off-by: Yi Wu <yiwu@pingcap.com>

update HISTORY.md

cac4936

Signed-off-by: Yi Wu <yiwu@pingcap.com>

yiwu-arbug requested review from siying and riversand963 October 11, 2019 22:05

facebook-github-bot added the CLA Signed label Oct 11, 2019

yiwu-arbug commented Oct 11, 2019

View reviewed changes

db/flush_job.h Outdated Show resolved Hide resolved

Yi Wu added 5 commits October 14, 2019 18:32

Fix lite build

73d954b

Signed-off-by: Yi Wu <yiwu@pingcap.com>

Merge remote-tracking branch 'origin/master' into flush_fix

7f07287

use unique_ptr install of raw pointer

591d4df

Signed-off-by: Yi Wu <yiwu@pingcap.com>

Merge branch 'master' into flush_fix

1904df1

revert unwanted changes

b52a2a9

Signed-off-by: Yi Wu <yiwu@pingcap.com>

riversand963 reviewed Oct 14, 2019

View reviewed changes

db/db_flush_test.cc Outdated Show resolved Hide resolved

db/db_flush_test.cc Outdated Show resolved Hide resolved

db/db_flush_test.cc Show resolved Hide resolved

db/db_flush_test.cc Outdated Show resolved Hide resolved

db/db_impl/db_impl.h Show resolved Hide resolved

facebook-github-bot reviewed Oct 14, 2019

View reviewed changes

Fix flush_job_test. Address comments

9d1a9b4

Signed-off-by: Yi Wu <yiwu@pingcap.com>

Merge branch 'master' into flush_fix

edf7137

update sync point name

0467f88

Signed-off-by: Yi Wu <yiwu@pingcap.com>

riversand963 approved these changes Oct 14, 2019

View reviewed changes

facebook-github-bot reviewed Oct 15, 2019

View reviewed changes

attempt to fix lint

e7d7709

Signed-off-by: Yi Wu <yiwu@pingcap.com>

facebook-github-bot reviewed Oct 15, 2019

View reviewed changes

Address lint error

05fd54f

Assert `file_meta` is non-empty before accessing it via subscription.

facebook-github-bot reviewed Oct 15, 2019

View reviewed changes

Merge branch 'master' into flush_fix

7befb2f

facebook-github-bot reviewed Oct 16, 2019

View reviewed changes

facebook-github-bot closed this in 1f9d7c0 Oct 16, 2019

yiwu-arbug deleted the flush_fix branch October 16, 2019 17:45

yiwu-arbug mentioned this pull request Oct 16, 2019

Fix OnFlushCompleted fired before flush result write to MANIFEST (#5908) tikv/rocksdb#127

Merged

facebook-github-bot added the Merged label Oct 16, 2019

pdillinger mentioned this pull request Nov 8, 2019

Test DBFlushTest.FireOnFlushCompletedAfterCommittedResult hangs sometimes #6017

Open

yiwu-arbug mentioned this pull request Nov 19, 2019

Fix OnFlushCompleted fired before flush result write to MANIFEST (#59… tikv/rocksdb#130

Merged

riversand963 mentioned this pull request Jul 24, 2021

Fix NotifyOnFlushCompleted() for atomic flush #8585

Closed

empiredan mentioned this pull request Dec 21, 2021

Fix NotifyOnFlushCompleted() for atomic flush (#8585) XiaoMi/pegasus-rocksdb#34

Merged

riversand963 mentioned this pull request Jul 13, 2022

Add a UT to trigger flush race #10334

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix OnFlushCompleted fired before flush result write to MANIFEST #5908

Fix OnFlushCompleted fired before flush result write to MANIFEST #5908

yiwu-arbug commented Oct 11, 2019 •

edited

Loading

riversand963 left a comment

facebook-github-bot left a comment

facebook-github-bot commented Oct 14, 2019

facebook-github-bot commented Oct 14, 2019

facebook-github-bot commented Oct 14, 2019

riversand963 left a comment

facebook-github-bot left a comment

yiwu-arbug commented Oct 15, 2019

riversand963 commented Oct 15, 2019

facebook-github-bot commented Oct 15, 2019

facebook-github-bot left a comment

riversand963 commented Oct 15, 2019

yiwu-arbug commented Oct 15, 2019

facebook-github-bot commented Oct 15, 2019

facebook-github-bot left a comment

facebook-github-bot commented Oct 16, 2019

facebook-github-bot left a comment

riversand963 commented Oct 16, 2019

yiwu-arbug commented Oct 16, 2019

facebook-github-bot commented Oct 16, 2019

Fix OnFlushCompleted fired before flush result write to MANIFEST #5908

Fix OnFlushCompleted fired before flush result write to MANIFEST #5908

Conversation

yiwu-arbug commented Oct 11, 2019 • edited Loading

riversand963 left a comment

Choose a reason for hiding this comment

facebook-github-bot left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Oct 14, 2019

facebook-github-bot commented Oct 14, 2019

facebook-github-bot commented Oct 14, 2019

riversand963 left a comment

Choose a reason for hiding this comment

facebook-github-bot left a comment

Choose a reason for hiding this comment

yiwu-arbug commented Oct 15, 2019

riversand963 commented Oct 15, 2019

facebook-github-bot commented Oct 15, 2019

facebook-github-bot left a comment

Choose a reason for hiding this comment

riversand963 commented Oct 15, 2019

yiwu-arbug commented Oct 15, 2019

facebook-github-bot commented Oct 15, 2019

facebook-github-bot left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Oct 16, 2019

facebook-github-bot left a comment

Choose a reason for hiding this comment

riversand963 commented Oct 16, 2019

yiwu-arbug commented Oct 16, 2019

facebook-github-bot commented Oct 16, 2019

yiwu-arbug commented Oct 11, 2019 •

edited

Loading