Rewrite state loading path on startup #4166
Conversation
marta-lokhova
requested review from
dmkozh,
SirTyson and
graydon
and removed request for
dmkozh and
SirTyson
src/ledger/LedgerManager.h
Outdated
| @@ -147,8 +147,8 @@ class LedgerManager | |||
| virtual void startNewLedger() = 0; | |||
|
|
|||
| // loads the last ledger information from the database | |||
| // if handler is set, also loads bucket information and invokes handler. | |||
| virtual void loadLastKnownLedger(std::function<void()> handler) = 0; | |||
| virtual void loadLastKnownLedger(bool startServices, | |||
There was a problem hiding this comment.
Nit: It would be helpful to add what the parameters mean to this comment. startServices is pretty clear, ledgerStateReady is not obvious unless you read the implementation. Maybe slightly rewording to isLedgerStateReady would help as well.
src/main/ApplicationImpl.cpp
Outdated
| } | ||
| }; | ||
| mLedgerManager->loadLastKnownLedger(loadConfig); | ||
| mLedgerManager->loadLastKnownLedger(false, true); |
There was a problem hiding this comment.
Nit: add /*startSevices=*/ and /*ledgerStateReady=*/ so we don't have back to back to back raw bools. Applies in a few other places as well.
|
|
||
| while (!done && mVirtualClock.crank(true)) |
There was a problem hiding this comment.
Wow this old interface was.... not good
src/ledger/LedgerManagerImpl.cpp
Outdated
|
|
||
| // Step 3. Restore BucketList if we're doing a full core startup | ||
| // (startServices=true), OR when using BucketListDB | ||
| if (startServices || mApp.getConfig().EXPERIMENTAL_BUCKETLIST_DB) |
There was a problem hiding this comment.
We should never check the raw flag, use getConfig().isUsingBucketListDB() instead.
src/ledger/LedgerManagerImpl.cpp
Outdated
| } | ||
| handler(); | ||
| // Work should only fail during graceful shutdown | ||
| releaseAssert(mApp.isStopping()); |
There was a problem hiding this comment.
Should this be releaseAssertOrThrow?
Addressed your feedback. Thanks for reviewing! |
src/ledger/LedgerManagerImpl.cpp
Outdated
| } | ||
| } | ||
|
|
||
| // Step 3. Restore LedgerManager's internal state |
src/ledger/LedgerManagerImpl.cpp
Outdated
| return; | ||
| } | ||
|
|
||
| // Step 4. If ledger state is ready and core is in v20, load network configs |
src/ledger/LedgerManagerImpl.cpp
Outdated
| // In some modes, e.g. in-memory, core's state is rebuilt asynchronously via | ||
| // catchup. In this case, we're not able to load the network config at this | ||
| // time, and instead must let catchup do it when ready. | ||
| if (!isLedgerStateReady) |
There was a problem hiding this comment.
I would rather change this to positive condition (i.e. if (isLedgerStateReady) { loadConfig() } else { LOG(...) } and get rid of the early return (it seems quite easy to forget about it and accidentally skip some logic independent of the config loading).
There was a problem hiding this comment.
ok, updated (fixed comments too)
marta-lokhova
force-pushed
the
startup_path
branch
from
8df0e4f to
b57d301
Compare
|
r+ b57d301 |
Resolves #4163