rsz: Support nested journals' move result tracking#10431
Conversation
Signed-off-by: Eren Dogan <erendogan@google.com>
Signed-off-by: Eren Dogan <erendogan@google.com>
Signed-off-by: Eren Dogan <erendogan@google.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements nested journaling support in the MoveCommitter by replacing the single journal flag with a stack-based tracking system for move results. This change allows for hierarchical commit and restore operations, aligning with the underlying database's ECO journal behavior. The feedback identifies a critical safety issue in the unrecording of move results where a multiset iterator must be validated before erasure to prevent potential crashes. Additionally, the reviewer pointed out that the new internal logic in the journaling methods makes several manual calls to accept or reject pending moves in SetupLegacyBase.cc redundant.
| pending_instances_by_type_[index].erase( | ||
| pending_instances_by_type_[index].find(inst)); |
There was a problem hiding this comment.
The result of find(inst) must be checked against end() before being passed to erase(). Passing end() to erase() is undefined behavior and will likely cause a crash if the instance is not found in the multiset for any reason.
auto it = pending_instances_by_type_[index].find(inst);
if (it != pending_instances_by_type_[index].end()) {
pending_instances_by_type_[index].erase(it);
}![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Signed-off-by: Eren Dogan <erendogan@google.com>
|
clang-tidy review says "All clean, LGTM! 👍" |
|
@codex review |
|
@erendn Could you please add a regression to capture the behavior? |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a0e5e8de7d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (pending_move_results_.empty()) { | ||
| // Outermost restore: the tracker's pending entries are no longer | ||
| // load-bearing. Flush them and emit the totals snapshot. | ||
| rejectTrackedMoves(); |
There was a problem hiding this comment.
During nested restores, this code only calls rejectTrackedMoves() when unwinding the outermost journal, so inner-branch tracker events remain in MoveTracker pending state even after their ECOs are undone. Those stale events are later finalized by the outer journal and get reported as if they belonged to the surviving branch, which corrupts move-tracker commit/reject reporting whenever nested journals are used.
Useful? React with 👍 / 👎.
Signed-off-by: Eren Dogan <erendogan@google.com>
Signed-off-by: Eren Dogan <erendogan@google.com>
|
clang-tidy review says "All clean, LGTM! 👍" |
Signed-off-by: Eren Dogan <erendogan@google.com>
|
clang-tidy review says "All clean, LGTM! 👍" |
|
I added a gtest to capture the behavior. It catches the current behavior, which is incorrect, but I left warnings to myself that when I actually fix it in refactor, the unit tests will make sure it behaves correctly. |
2 participants
Summary
MoveCommitterdoesn't exactly use nested journaling and can leave footprints of unrolled changes behind (hasMoves()sees moves on instances that were reverted). This PR doesn't fix it yet since it will change the resizer behavior but I will fix it during the refactor. After this PR is merged, that fix is just a one-liner.Type of Change
Impact
It doesn't change the behavior just yet.
Verification
./etc/Build.sh).