Turn titan into read-only on error during gc and version_change #42
Conversation
JiayuZzz
commented
Jul 14, 2019
JiayuZzz
commented
- Set background error if encountered a critical error. Now it's set for two cases
- BackgroundGC() error
- LogAndApply() error after generate new blob file
- If background error is set, all following foreground write operations will be refused and return background error status
Codecov Report
@@ Coverage Diff @@
## master #42 +/- ##
=========================================
+ Coverage 84.12% 84.33% +0.2%
=========================================
Files 43 43
Lines 2577 2617 +40
=========================================
+ Hits 2168 2207 +39
- Misses 409 410 +1 |
src/db_impl_gc.cc
Outdated
| Status bg_err = s; | ||
| if (!db_options_.listeners.empty()) { | ||
| // TODO(@jiayu) : check if mutex_ is freeable for futrue use case | ||
| mutex_.Unlock(); |
There was a problem hiding this comment.
I don't see why we could unlock here. Listener iterator needs syncing, and callback will mutate bg_err too.
There was a problem hiding this comment.
bg_err is a local variable, won't be accessed concurrently.
There was a problem hiding this comment.
I'm not sure if listeners would be modified, it seems no API to do that unless reopen db
src/titan_db_test.cc
Outdated
| @@ -748,6 +814,9 @@ TEST_F(TitanDBTest, FallbackModeEncounterMissingBlobFile) { | |||
| VerifyDB({{"bar", "v1"}}); | |||
| } | |||
|
|
|||
| TEST_F(TitanDBTest, BackgroundErrorHandling) { BackgroundErrorHandling(); } | |||
There was a problem hiding this comment.
Could we unfold the function here? Not general enough to be a test class method i think.
src/db_impl.h
Outdated
| @@ -171,6 +221,9 @@ class TitanDBImpl : public TitanDB { | |||
| EnvOptions env_options_; | |||
| DBImpl* db_impl_; | |||
| TitanDBOptions db_options_; | |||
| // Turn DB into read-only if background happened | |||
src/db_impl_gc.cc
Outdated
| mutex_.AssertHeld(); | ||
| Status bg_err = s; | ||
| if (!db_options_.listeners.empty()) { | ||
| // TODO(@jiayu) : check if mutex_ is freeable for futrue use case |
| @@ -155,6 +196,15 @@ class TitanDBImpl : public TitanDB { | |||
| return oldest_snapshot; | |||
| } | |||
|
|
|||
| Status SetBGError(const Status& s); | |||
There was a problem hiding this comment.
Should add mutex held hint. btw, google code style says we should write accessor as set_bg_error() and bg_error(), not sure if we follows.
There was a problem hiding this comment.
I think the rocksdb convention is do SetBgError for non-trivial assessor and set_bg_error for trivial ones. And this one is non-trivial.
tabokie
changed the title
| @@ -43,6 +43,47 @@ class TitanDBImpl : public TitanDB { | |||
|
|
|||
| Status CloseImpl(); | |||
|
|
|||
| using TitanDB::Put; | |||
There was a problem hiding this comment.
Move the following changes to db_impl.cc?
| @@ -155,6 +196,15 @@ class TitanDBImpl : public TitanDB { | |||
| return oldest_snapshot; | |||
| } | |||
|
|
|||
| Status SetBGError(const Status& s); | |||
There was a problem hiding this comment.
I think the rocksdb convention is do SetBgError for non-trivial assessor and set_bg_error for trivial ones. And this one is non-trivial.
| // BG error is restored by listener for first time | ||
| ASSERT_OK(db_->Put(WriteOptions(), key, val)); | ||
| SetBGError(Status::IOError("")); | ||
| ASSERT_OK(db_->Get(ReadOptions(), key, &val)); |
There was a problem hiding this comment.
Put a non-empty value and verify the value return from Get?
|
@tabokie thanks for reviewing btw :) |
|
@yiwu-arbug @tabokie All modified, thanks for reviewing. |
Turn titan into read-only on error during gc and version_change (tikv#42)
