LedgerManager, LedgerTxn tech debt #4725
Conversation
marta-lokhova
force-pushed
the
ledgerCloseTechDebt
branch
from
e053306 to
c9fb8ab
Compare
| if (mLedgerApplyManager.isApplying()) | ||
| { | ||
| trackingHeartBeat(); | ||
| return; | ||
| } |
There was a problem hiding this comment.
this branch on isApplying is new; it's an assert before. is this correct? I can't really follow all the places this gets triggered.
| .copySearchableLiveBucketListSnapshot(); | ||
| } | ||
| releaseAssert(threadIsMain()); | ||
| releaseAssert(getLCLState().snapshot); |
There was a problem hiding this comment.
Interesting! Did no callers of this rely on the previous lazy population?
| @@ -1496,12 +1348,8 @@ LedgerManagerImpl::maybeResetLedgerCloseMetaDebugStream(uint32_t ledgerSeq) | |||
| SearchableSnapshotConstPtr | |||
| LedgerManagerImpl::getLastClosedSnaphot() | |||
There was a problem hiding this comment.
| LedgerManagerImpl::getLastClosedSnaphot() | |
| LedgerManagerImpl::getLastClosedSnapshot() |
| ThreadInvariant::throwIfWrongThread() const | ||
| { | ||
| std::lock_guard<std::recursive_mutex> guard(mThreadInvariantMutex); | ||
| if (mActiveThreadIdSet && mThreadID != std::this_thread::get_id()) | ||
| { | ||
| throw std::runtime_error("ThreadInvariant called from wrong thread"); | ||
| } | ||
| } |
There was a problem hiding this comment.
I'm not complaining about this, but curious: taking a mutex and doing a TLS access on every single method in this class might show up in profiles, considering how hot the ltx sometimes is; does it?
| // `commit` and `rollback` terminate the program on an exception, | ||
| // because they are marked noexcept SECTION("commit child") |
There was a problem hiding this comment.
This bit is somewhat worrying. Or rather: I wonder if the thread-invariant error ought to just be an abort in all cases, rather than an exception? the ltx is exception safe because transactions run in an exception-catch block and use exceptions for "meaningfully recoverable" errors. But a thread-safety violation isn't really recoverable, right? It should probably always be fatal?
Resolves #4592