Details
-
Task
-
Resolution: Fixed
-
Major
-
7.1.0
-
1
Description
Affects KV Neo CV.
This development assertion:
bool CheckpointMemRecoveryTask::runInner() { |
...
|
case CheckpointRemoval::Eager: { |
#if CB_DEVELOPMENT_ASSERTS
|
// if eager checkpoint removal has been configured, calling |
// attemptCheckpointRemoval here should never, ever, find any |
// checkpoints to remove; they should always be removed as soon |
// as they are made eligible, before the lock is released. |
// This is not cheap to verify, as it requires scanning every |
// vbucket, so is only checked if dev asserts are on. |
Expects(attemptCheckpointRemoval().second == 0);
|
#endif
|
break; |
}
|
Was essentially expected to be a no-op, as Eager checkpoint removal ensures that no unreffed checkpoints remain in the checkpoint manager.
However,
CheckpointMemRecoveryTask::attemptCheckpointRemoval() {
|
...
|
memReleased +=
|
vb->checkpointManager->removeClosedUnrefCheckpoints().memory;
|
CheckpointManager::ReleaseResult
|
CheckpointManager::removeClosedUnrefCheckpoints() {
|
...
|
CheckpointList toRelease;
|
{
|
std::lock_guard<std::mutex> lh(queueLock);
|
maybeCreateNewCheckpoint(lh);
|
toRelease = extractClosedUnrefCheckpoints(lh);
|
}
|
It also has the potential side effect of closing the current open checkpoint. Once CB_DEVELOPMENT_ASSERTS was set to OFF for neo, this side effect would no longer occur.
ep_testsuite mem stats test fails if this side effect does not occur:
static enum test_result test_mem_stats(EngineIface* h) {
|
...
|
int itemsRemoved = get_int_stat(h, "ep_items_rm_from_checkpoints");
|
wait_for_persisted_value(h, "key", value.c_str());
|
testHarness->time_travel(65);
|
if (isPersistentBucket(h)) {
|
wait_for_stat_change(h, "ep_items_rm_from_checkpoints", itemsRemoved);
|
}
|
This will timeout, as nothing triggers a checkpoint close, so the checkpoint will not yet be removed routinely.
KV behaviour should not be visibly changed by changing CB_DEVELOPMENT_ASSERTS (aside from if the guarded assertion fails, of course).
Additionally, the test should not rely on this side effect.