[ZEPPELIN-6529] Evict note cache entries when removing a folder#5296
Open
HwangRock wants to merge 1 commit into
Open
[ZEPPELIN-6529] Evict note cache entries when removing a folder#5296HwangRock wants to merge 1 commit into
HwangRock wants to merge 1 commit into
Conversation
NoteManager.removeFolder removed the deleted notes from notesInfo and the in-memory folder tree, but did not call noteCache.removeNote for each note, so the note objects stayed in the NoteCache until the LRU threshold naturally evicted them. removeNote(String, AuthenticationInfo) already evicts the cache. Evict each removed note from noteCache in the same loop that clears notesInfo, mirroring the single-note removal path. This frees the heap held by deleted notes immediately and stops them from occupying cache slots that live notes could use, which matters most for large folder deletions such as emptying the trash. Added NoteManagerTest#testRemoveFolderEvictsNoteCache: adds two notes under a folder, asserts they are cached, removes the folder, and asserts the cache is emptied. Fails before the change (cache size stays 2), passes after.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What is this PR for?
NoteManager.removeFolderremoved the deleted notes fromnotesInfoand the in-memory folder tree, but never evicted them from theNoteCache.removeNote(String, AuthenticationInfo)already callsnoteCache.removeNote(noteId), so the single-note path is clean; the folder path was not. As a result theNoteobjects for deleted notes stayed on the heap until the LRU threshold naturally evicted them, and they kept occupying cache slots that live notes could otherwise use. This is most wasteful for large folder deletions such as emptying the trash.This PR evicts each removed note from
noteCachein the same loop that clearsnotesInfo, mirroring the single-note removal path. There is no functional/correctness change (processNotealready gates onnotesInfo.containsKey), only immediate reclamation of the cache slots and heap held by deleted notes.This was found by @ParkGyeongTae while reviewing #5288 / #5292 (ZEPPELIN-6345).
What type of PR is it?
Improvement
What is the Jira issue?
How should this be tested?
NoteManagerTest#testRemoveFolderEvictsNoteCache: adds two notes under/folder1, assertsgetCacheSize() == 2, callsremoveFolder("/folder1", ...), and assertsgetCacheSize() == 0. Fails before the change (cache size stays2), passes after.NoteManagerTestpasses (6 tests), includingtestLruCacheandtestNoteOperations.Questions: