Improve online delete backend lock #3342
Conversation
|
@miguelportilla, this branch is based off of the wrong 1.6.0-b1; your commit needs to apply on top of 4f422f6. |
miguelportilla
force-pushed
the
rotate_fix
branch
from
82ecab9 to
8b6ad7a
Compare
| @@ -102,19 +148,49 @@ DatabaseRotatingImp::sweep() | |||
| std::shared_ptr<NodeObject> | |||
| DatabaseRotatingImp::fetchFrom(uint256 const& hash, std::uint32_t seq) | |||
| { | |||
| Backends b = getBackends(); | |||
| auto nObj = fetchInternal(hash, b.writableBackend); | |||
| auto backends = [&] { | |||
There was a problem hiding this comment.
Might be a good place for a structured binding (here and in the similar lambda in for_each). But fine as-is as well.
There was a problem hiding this comment.
Good idea, thanks!
There was a problem hiding this comment.
👍 but holding off on official signoff for a response to https://github.com/ripple/rippled/pull/3342/files#r409703104
miguelportilla
force-pushed
the
rotate_fix
branch
from
8b6ad7a to
e7c76c4
Compare
| savedState.lastRotated = lastRotated_; | ||
| state_db_.setState(savedState); | ||
|
|
||
| JLOG(journal_.warn()) << "finished rotation " << validatedSeq; |
There was a problem hiding this comment.
The previous locking behavior needs to be there for rotateBackends() and for clearCaches()--the same lock needs to cover them both. For the fullbelow cache, each call to clear() increments an integer called "m_gen" (generation). The generation determines if the cache entry is valid for the existing backends, or if it applies to backends that just rotated.
| { | ||
| auto oldBackend {std::move(archiveBackend_)}; | ||
| std::lock_guard lock(mutex_); |
There was a problem hiding this comment.
Should be injected by the caller, described above.
| savedState.archiveDb = dbRotating_->rotateBackends( | ||
| std::move(newBackend)); | ||
| savedState.lastRotated = lastRotated_; | ||
| state_db_.setState(savedState); |
There was a problem hiding this comment.
setState() should come before rotateBackends(), as in previous behavior. In the event of process crash after setState() but before rotateBackends(), then upon process startup, the correct Backends would be in place because the state database would reflect that, even if they hadn't been rotated within the process before the crash. However, with the new behavior, if the process crashes after rotation, but before the Backend names (filepthss) are persisted, then upon process restart, rippled will expect to see the old Backends.
As for the lock, in previous behavior, setState() and rotateBackend() were both called within a lock. It looks safe to now call setState() without being locked together with rotateBackend().
There was a problem hiding this comment.
Curious: could we "wrap" this logic inside a TRANSACTION?
There was a problem hiding this comment.
If you mean a SQLIte transaction, no. Because rotateBackends() is not involved with SQLite, but affects the Backend objects that write to the nodestore.
There was a problem hiding this comment.
@mtrippled Good point, addressed.
There was a problem hiding this comment.
Conditional approval, based on @mtrippled's concerns being addressed.
miguelportilla
force-pushed
the
rotate_fix
branch
2 times, most recently
from
f66e258 to
0d262e6
Compare
miguelportilla
force-pushed
the
rotate_fix
branch
from
0d262e6 to
c2541ad
Compare
Codecov Report
@@ Coverage Diff @@
## develop #3342 +/- ##
===========================================
- Coverage 70.23% 70.21% -0.02%
===========================================
Files 683 683
Lines 54630 54648 +18
===========================================
+ Hits 38370 38372 +2
- Misses 16260 16276 +16
Continue to review full report at Codecov.
|
miguelportilla
added
the
Ready to merge
Addresses a possibility in the online delete process where one or more backend shared pointer references may become invalid during rotation.