HwangRock opened a new pull request, #5325:
URL: https://github.com/apache/zeppelin/pull/5325
### What is this PR for?
`moveNote` mutates the folder tree, the `notesInfo` mapping and the notebook
repo without holding any lock, while `saveNote` derives its target file name
from the path carried by the `Note` object at save time. A save racing with a
move therefore writes the note back to its pre-move path, and the same noteId
ends up on disk twice. ZEPPELIN-5858 describes exactly this and ships a
reproduction; this PR ports that reproduction onto the current API and fixes
the race.
Reproduced on current master with the ported test
(`VFSNotebookRepoWithDelay` injects repo latency, e.g. remote storage):
```
Saving note 2MXKZT37A to folder_1/note_2MXKZT37A.zpln
Move note 2MXKZT37A to /folder_2/note
Move note 2MXKZT37A from /folder_1/note to /folder_2/note
Saving note 2MXKZT37A to folder_1/note_2MXKZT37A.zpln <- stale path,
resurrects the old file
Duplicate note ids detected. .zpln files under notebookDir:
[folder_2/note_2MXKZT37A.zpln, folder_1/note_2MXKZT37A.zpln]
java.lang.IllegalArgumentException: Duplicate note ids
```
The fix has two parts, both inside the existing `saveNote` monitor so mutual
exclusion actually holds against the already-synchronized save path:
- `saveNote` realigns the note path from `notesInfo` (the authoritative id
-> path mapping) before deriving the file name. Whichever side wins the
monitor, a save always lands on the note's current location.
- `moveNote`, `removeNote` and `moveFolder` now perform their tree, mapping
and repo mutations under that same monitor.
One deliberate detail: the fixed lock order in this code base is note
`readLock` -> NoteManager monitor, because every save caller runs inside
`processNote` holding the note's readLock. `moveNote` therefore updates the
cached note path directly via the note cache instead of calling `processNote`
while holding the monitor, and the rename-triggered resave stays outside the
monitor and reuses `saveNote`. This keeps the lock order consistent on every
path and avoids introducing an inversion.
Verified the causality both ways: the reproduction test fails with
`Duplicate note ids` on master, passes with this change, and fails again if
only the `NoteManager` change is reverted.
### What type of PR is it?
Bug Fix
### Todos
* [x] Port the reproduction attached to ZEPPELIN-5858 onto the current
NotebookRepo API
* [x] Serialize NoteManager mutations and realign the note path before save
### What is the Jira issue?
https://issues.apache.org/jira/browse/ZEPPELIN-5858
### How should this be tested?
`mvn test -pl zeppelin-server -Dtest=NotebookServiceRaceConditionTest` — the
test runs `renameNote` and `insertParagraph` concurrently against a
delay-injecting `VFSNotebookRepo` and fails with `Duplicate note ids` without
the `NoteManager` change.
Regression: `NoteManagerTest`, `NotebookServiceTest`, `NotebookTest` (52
tests) all pass.
### Questions:
* Does the license files need to update? No
* Is there breaking changes for older versions? No
* Does this needs documentation? No
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
