[ZEPPELIN-6579] Make notebook tree reload safe for concurrent note operations

[big-cir]

What is this PR for?

NoteManager locates a note through two separate pieces of state: notesInfo maps a note id to its path, and root holds the folder tree that the path is walked against. A lookup uses both in sequence, so the two have to agree.

reloadNotes() replaced them one at a time:

public void reloadNotes() throws IOException {
  this.root = new Folder("/", notebookRepo, noteCache, zConf);  // (1) tree becomes empty
  this.trash = this.root.getOrCreateFolder(TRASH_FOLDER);
  init();                                                      // (2) new mapping, (3) refill tree
}

Neither field is volatile and nothing is held while they are swapped, so a concurrent processNote() can observe a mapping and a tree that belong to different generations:

time	reloading thread	note request thread	state
t1	installs an empty tree		mapping: old (complete) / tree: empty
t2		notesInfo.containsKey(noteId) passes	the id is still in the old mapping
t3		walks the path in the tree, finds nothing	throws
t4	installs the new mapping
t5	refills the tree, one note at a time		notes not inserted yet still fail

The guard in processNote() only checks notesInfo, so it passes and the failure surfaces one line later in getNoteNode():

java.io.IOException: Can not find note: /E2E_TEST_FOLDER/TestNotebook_...
  at org.apache.zeppelin.notebook.NoteManager.getNoteNode
  at org.apache.zeppelin.notebook.NoteManager.processNote
  at org.apache.zeppelin.rest.NotebookRestApi.updateParagraph

IOException is not mapped to a specific status, so WebApplicationExceptionMapper turns it into HTTP 500 for a note that was never removed. Everything that goes through processNote() is affected: reading a note, updating a paragraph, creating, deleting and moving notes, and listing the notebook.

This PR holds the tree, the trash folder and the mapping in one immutable NoteTree and publishes it with a single volatile write. buildNoteTree() fills the new tree locally and returns it; only then is it assigned. The tree-walking helpers (getNoteNode, getFolder, getOrCreateFolder, isNotePathAvailable) take the tree as a parameter, and callers that need both pieces of state read the reference once, so a lookup resolves the mapping and the tree against the same generation. Those helpers are static so that the compiler prevents them from reaching back to the field.

Scope and related issues

#5325 ([ZEPPELIN-5858]) is open against the same class and restructures removeNote, moveNote and moveFolder with synchronized (this). It targets a different race (two mutators duplicating a note) and its monitor does not cover reloadNotes(), so neither change subsumes the other. Whichever merges second will need a rebase.

What type of PR is it?

Bug Fix

Todos

• - Build the new tree, trash folder and mapping in buildNoteTree() before publishing them
• - Hold the three in an immutable NoteTree published through a single volatile write
• - Pass the tree into the tree-walking helpers so one lookup uses one generation
• - Add a regression test that reloads while other threads read notes
• - Confirm the test fails without the fix and passes with it

What is the Jira issue?

• ZEPPELIN-6579

How should this be tested?

New test NoteManagerTest#testConcurrentReloadAndProcessNote: it saves 50 notes, then runs reloadNotes() in a loop on one thread while four threads keep calling processNote() for every note, and asserts that no lookup fails or returns nothing.

export JAVA_HOME=$(/usr/libexec/java_home -v 11)
./mvnw package -pl zeppelin-server --am -Dtest=NoteManagerTest -DfailIfNoTests=false

Result with the fix: Tests run: 7, Failures: 0, Errors: 0.

Reverting only the production change makes the new test fail on every reader thread with java.io.IOException: Can not find note: /prod/note_0 thrown from NoteManager.getNoteNode via NoteManager.processNote, which is the stack from the ticket; with the fix it passes.

Also run, to cover the callers of the reload path:

./mvnw package -pl zeppelin-server --am \
  -Dtest='NotebookTest#testReloadAllNotes+testReloadAndSetInterpreter' -DfailIfNoTests=false

Result: Tests run: 2, Failures: 0, Errors: 0.

Not verified locally: the full NotebookTest and NotebookServerTest classes, which start real remote interpreter processes and time out in my environment, and NotebookRepoSyncTest. Those are left to CI.

Screenshots (if appropriate)

N/A

Questions:

• Does the license files need to update? No
• Is there breaking changes for older versions? No
• Does this needs documentation? No

[jongyoul]

Thanks for fixing the mixed-generation lookup failure. I narrowed the title to match this focused scope: atomically publishing one fully built notebook-tree state during reload.

Please address writer-side coordination in a follow-up PR: concurrent reloads, and reload racing with add/save/move/remove. That follow-up should include deterministic latch-based tests and account for the overlap with #5325. The current approval remains for this focused fix.

[big-cir]

Please address writer-side coordination in a follow-up PR: concurrent reloads, and reload racing with add/save/move/remove. That follow-up should include deterministic latch-based tests and account for the overlap with #5325. The current approval remains for this focused fix.

Thanks for the review!!

Should I open a follow-up JIRA ticket for writer-side coordination (concurrent reloads and race conditions with add/save/move/remove) and handle it in a separate PR?

[jongyoul]

Merged into master (d8c43cb).