Use ConditionalWork for history replay in catchup #2194
Conversation
marta-lokhova
force-pushed
the
ledger_replay_conditional_work
branch
from
7800db1
to
e1cd566
Compare
| if (mWorkStarted && !mConditionedWork->isDone()) | ||
| { | ||
| mConditionedWork->crankWork(); | ||
| return false; |
There was a problem hiding this comment.
Does that mean that aborting started ConditionalWork is not possible?
There was a problem hiding this comment.
Not sure what you mean: onAbort returns whether Work is done aborting or not, so that BasicWork can proceed appropriately. ConditionalWork will keep getting scheduled to run onAbort until mConditionedWork is done aborting. At that point, ConditionalWork is safe to abort itself.
| // Conditioned work was aborted | ||
| REQUIRE(conditionedWork->getState() == BasicWork::State::WORK_ABORTED); | ||
| // Dependent work was never started (internally, in PENDING state, | ||
| // enforced by its destructor) |
There was a problem hiding this comment.
I think you mean constructor here?
There was a problem hiding this comment.
No, I mean destructor, which asserts that BasicWork is in proper state.
src/work/test/WorkTests.cpp
Outdated
| { | ||
| clock.crank(); | ||
| } | ||
| CHECK(conditionedWork->getState() == BasicWork::State::WORK_WAITING); |
src/catchup/ApplyCheckpointWork.cpp
Outdated
| mHdrIn.close(); | ||
| mTxIn.close(); | ||
| mFilesOpen = false; | ||
| } | ||
|
|
||
| void | ||
| ApplyLedgerChainWork::openCurrentInputFiles() | ||
| ApplyCheckpointWork::openCurrentInputFiles() |
There was a problem hiding this comment.
Could be renamed to openInputFiles. If this only works on one checkpoint there is only one set of files, so "current" is redundant.
| @@ -45,7 +45,6 @@ BatchWork::doWork() | |||
| } | |||
|
|
|||
| addMoreWorkIfNeeded(); | |||
| mApp.getCatchupManager().logAndUpdateCatchupStatus(true); | |||
src/catchup/ApplyCheckpointWork.h
Outdated
| LedgerRange const mRange; | ||
| uint32_t mCurrSeq; | ||
| uint32_t mCheckpoint; | ||
| // Apply checkpoint until given ledger. Otherwise, apply checkpoint in full |
There was a problem hiding this comment.
pick a name that clarifies that this is a ledger number
src/catchup/ApplyCheckpointWork.h
Outdated
| LedgerHeaderHistoryEntry& lastApplied); | ||
| ~ApplyLedgerChainWork() = default; | ||
| ApplyCheckpointWork(Application& app, TmpDir const& downloadDir, | ||
| uint32_t checkpoint, uint32_t boundary = 0); |
There was a problem hiding this comment.
I don't like the use of 0 like this in a low level class as it's quite error prone.
I think it's probably better to keep a LedgerRange here, and force the caller to make a decision on what the upper range is.
This PR then just needs to enforce that that LedgerRange has the following properties:
- low is a checkpoint
- doesn't span multiple checkpoints
There was a problem hiding this comment.
Agreed that it's safer, updated.
marta-lokhova
force-pushed
the
ledger_replay_conditional_work
branch
2 times, most recently
from
20e3508
to
2063642
Compare
src/catchup/ApplyCheckpointWork.cpp
Outdated
| { | ||
| return State::WORK_SUCCESS; | ||
| mApp.getCatchupManager().logAndUpdateCatchupStatus(true); |
There was a problem hiding this comment.
I'd call logAndUpdateCatchupStatus regardless (before the if) - it seems arbitrary to only call it if result is true.
There was a problem hiding this comment.
I would also move the done case to its own it (probably done first) as to make the code easier to read (else will be deciding between running or failure)
marta-lokhova
force-pushed
the
ledger_replay_conditional_work
branch
from
2063642
to
ad674d6
Compare
|
r+ ad674d6 |
…_work Use ConditionalWork for history replay in catchup Reviewed-by: MonsieurNicolas
Resolves #1951