-
Notifications
You must be signed in to change notification settings - Fork 72
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
WC: fix for stop while shutting down #499
Conversation
@udi-speedb , plz wait with the review since changing CancelAllBackgroundWork to Close seemed to reveal a new issue which is handled in #500 . |
3349f79
to
2ffee5b
Compare
also need to include a notify on the write controllers condition variable when there a BG error (SetBGError) |
2e4894b
to
c4efc92
Compare
@udi-speedb , plz review. issue #500 is still unresolved as it requires further discussion so we can proceed with this PR IMO. |
@Yuval-Ariel - Approving. |
yes i did |
@erez-speedb , plz check that theres no degradation. thanks |
performance - no degradation. |
Also switch to waiting a sec on the CV each time. This is required since a bg error doesn't signal the CV in the WriteController.
fd1a5c6
to
37b91b6
Compare
Also switch to waiting a sec on the CV each time. This is required since a bg error doesn't signal the CV in the WriteController.
Also switch to waiting a sec on the CV each time. This is required since a bg error doesn't signal the CV in the WriteController.
as part of facebook/rocksdb#10765 , a check of whether the waiting db was shutting_down_ was intoduced.
The write thread then stops waiting when shutting_down_ is turned on which is the case when CancelAllBackgroundWork is called. But the rebased code now waits forever since #346 replaced the waiting on bg_cv_ to a wait on a write controller condition variable inside WriteController::WaitOnCV.
The condition variable inside WaitOnCV (stop_cv_) is only notified through the detor of a StopWriteToken.
The write_controller_token_ in each CF is being reset in any case after the call to CancelAllBackgroundWork from DBImpl::CloseHelper() (detor of DBImpl) so if the test would issue a db Close and then wait for the thread there would be no problem.
also consider fixing by instead of replacing CancelAllBackgroundWork with Close, to add a function right after the call to CancelAllBackgroundWork which notifies the stop_cv_ . this will make it clear to further unit test writers that calling CancelAllBackgroundWork is not sufficient in order to remove the stop condition