OAK-12357 - CheckpointCompactor: diff retry cycles against the previous uncompacted root - #3082
OAK-12357 - CheckpointCompactor: diff retry cycles against the previous uncompacted root#3082smiroslav wants to merge 1 commit into
Conversation
…us uncompacted root
Commit-Check ✔️ |
|
jsedding
left a comment
jsedding
left a comment
There was a problem hiding this comment.
@smiroslav very good find! Makes perfect sense. Also your test case is excellent! 👍
I left two comments. Mostly, I was wondering if we could make the code even more clear in the hope that we can avoid future regressions. Of course the unit-tests already get us there to a significant degree. So I'll leave it up to you to decide.
Thanks for fixing this regression! 🙂
| // Nodes a correct retry may compact beyond the changed children (their ancestor spine plus the | ||
| // concurrent-checkpoint structure). |
There was a problem hiding this comment.
I don't understand this comment.
Also, what does "spine" mean in this context?
| CompactedNodeState compacted = null; | ||
| // The previously processed super-root's *uncompacted* root, used as the diff base for the next | ||
| // super-root. It is in the same GC generation as the state being compacted, so MapRecord bucket | ||
| // pruning (record-id based) can skip unchanged subtrees. Using the compacted result here instead | ||
| // (a different, target generation) would defeat that pruning and force a full-tree traversal on | ||
| // every retry cycle. | ||
| NodeState previousAfterRoot = null; | ||
| for (String path : superRoots) { | ||
| NodeState afterSuperRoot = getDescendant(after, path, NodeState::getChildNode); | ||
| NodeState afterRoot = getRoot(afterSuperRoot); | ||
|
|
||
| NodeState baseRoot = requireNonNullElseGet(compacted, () -> getRoot(before)); | ||
| // diff base: the previous uncompacted root (same generation as afterRoot); falls back to `before` | ||
| // for the first super-root. NOT `compacted`, which is in the target generation. | ||
| NodeState baseRoot = requireNonNullElseGet(previousAfterRoot, () -> getRoot(before)); | ||
| // apply target: the previously compacted result, so the output stays fully compacted; falls back | ||
| // to `onto` for the first super-root. | ||
| NodeState ontoRoot = requireNonNullElseGet(compacted, () -> getRoot(onto)); | ||
|
|
||
| compacted = compactRootState(baseRoot, getRoot(afterSuperRoot), ontoRoot, hardCanceller, softCanceller); | ||
| compacted = compactRootState(baseRoot, afterRoot, ontoRoot, hardCanceller, softCanceller); | ||
| if (compacted == null) { | ||
| // only happens for hard cancellation | ||
| return null; | ||
| } | ||
|
|
||
| Validate.checkState(compacted.isComplete() || isCancelled(softCanceller), | ||
| "compaction must be complete unless cancelled"); | ||
|
|
||
| NodeBuilder builder = getDescendant(rootBuilder, path, NodeBuilder::child); | ||
| builder.setChildNode(ROOT, compacted); | ||
| if (path.startsWith(CHECKPOINTS + '/')) { | ||
| compactCheckpointMetadata(builder, afterSuperRoot); | ||
| } | ||
|
|
||
| previousAfterRoot = afterRoot; | ||
|
|
||
| if (isCancelled(softCanceller)) { | ||
| break; | ||
| } | ||
| } |
There was a problem hiding this comment.
Your code is perfectly correct. I was wondering, however, how the intention of the code could be expressed more clearly. Below is my attempt at this.
In words: to compact each checkpoint (and in the end head-state), we iterate over the superRoots. The root states of the checkpoints are replayed sequentially, in order to maximise the reused records in each compacted ontoRoot.
So for each iteration the previous afterRoot becomes the new beforeRoot, and the previous compacted state becomes the new ontoRoot.
For the afterRoot, the state for the current iteration is always retrieved from the current checkpoint being processed.
WDYT? Clearly, whether or not the code is intuitive is subjective. So feel free to proceed as you like.
NodeState baseRoot = getRoot(before);
NodeState ontoRoot = getRoot(onto);
for (String path : superRoots) {
NodeState afterSuperRoot = getDescendant(after, path, NodeState::getChildNode);
NodeState afterRoot = getRoot(afterSuperRoot);
CompactedNodeState compacted = compactRootState(baseRoot, afterRoot, ontoRoot, hardCanceller, softCanceller);
if (compacted == null) {
// only happens for hard cancellation
return null;
}
Validate.checkState(compacted.isComplete() || isCancelled(softCanceller),
"compaction must be complete unless cancelled");
NodeBuilder builder = getDescendant(rootBuilder, path, NodeBuilder::child);
builder.setChildNode(ROOT, compacted);
if (path.startsWith(CHECKPOINTS + '/')) {
compactCheckpointMetadata(builder, afterSuperRoot);
}
if (isCancelled(softCanceller)) {
break;
}
baseRoot = afterRoot; // afterRoot is the baseRoot of the next iteration
ontoRoot = compacted;
}
2 participants
https://issues.apache.org/jira/browse/OAK-12357