[GitHub] [zeppelin] Reamer commented on a change in pull request #4252: [ZEPPELIN-5559] Add note cache

[GitBox <gi...@apache.org>]

Reamer commented on a change in pull request #4252:
URL: https://github.com/apache/zeppelin/pull/4252#discussion_r780078820



##########
File path: zeppelin-zengine/src/main/java/org/apache/zeppelin/notebook/Notebook.java
##########
@@ -352,43 +355,64 @@ public void removeCorruptedNote(String noteId, AuthenticationInfo subject) throw
     authorizationService.removeNoteAuth(noteId);
   }
 
+  /**
+   * Removes the Note with the NoteId.
+   * @param noteId
+   * @param subject
+   * @throws IOException when fail to get it from NotebookRepo
+   */
   public void removeNote(String noteId, AuthenticationInfo subject) throws IOException {
-    Note note = getNote(noteId);
-    removeNote(note, subject);
+    processNote(noteId,
+      note -> {
+        if (note != null) {
+          removeNote(note, subject);
+        }
+        return null;
+    });
   }
 
   /**
-   * Get note from NotebookRepo and also initialize it with other properties that is not
-   * persistent in NotebookRepo, such as paragraphJobListener.
+   * Process a note in an eviction aware manner. It initializes the note
+   * with other properties that is not persistent in NotebookRepo, such as paragraphJobListener.
+   * <p>
+   * Use {@link #processNote(String, boolean, NoteProcessor)} in case you want to force
+   * a note reload from the {@link #NotebookRepo}.
+   * </p>
    * @param noteId
-   * @return null if note not found.
-   * @throws IOException when fail to get it from NotebookRepo.
+   * @param noteProcessor
+   * @return result of the noteProcessor
+   * @throws IOException when fail to get it from {@link NotebookRepo}
    */
-  public Note getNote(String noteId) throws IOException {
-    return getNote(noteId, false);
+  public <T> T processNote(String noteId, NoteProcessor<T> noteProcessor) throws IOException {
+    return processNote(noteId, false, noteProcessor);
   }
 
   /**
-   * Get note from NotebookRepo and also initialize it with other properties that is not
-   * persistent in NotebookRepo, such as paragraphJobListener.
+   * Process a note in an eviction aware manner. It initializes the note
+   * with other properties that is not persistent in NotebookRepo, such as paragraphJobListener.
+   *
    * @param noteId
-   * @return null if note not found.
-   * @throws IOException when fail to get it from NotebookRepo.
+   * @param reload
+   * @param noteProcessor
+   * @return result of the noteProcessor
+   * @throws IOException when fail to get it from {@link NotebookRepo}
    */
-  public Note getNote(String noteId, boolean reload) throws IOException {
-    Note note = noteManager.getNote(noteId, reload);
-    if (note == null) {
-      return null;
-    }
-    note.setInterpreterFactory(replFactory);
-    note.setInterpreterSettingManager(interpreterSettingManager);
-    note.setParagraphJobListener(paragraphJobListener);
-    note.setNoteEventListeners(noteEventListeners);
-    note.setCredentials(credentials);
-    for (Paragraph p : note.getParagraphs()) {
-      p.setNote(note);
-    }
-    return note;
+  public <T> T processNote(String noteId, boolean reload, NoteProcessor<T> noteProcessor) throws IOException {
+    return noteManager.processNote(noteId, reload, note -> {
+      if (note == null) {
+        return noteProcessor.process(note);

Review comment:
       Yes, for several reasons.
   
   To return a correct response as in this [case](https://github.com/apache/zeppelin/pull/4252/files#diff-017bf51cce0cfa2d227c2a93230a2164bbad6b98de581a54dc4a778b49704497R128).
   
   ```java
   if (note == null) {
     return new JsonResponse<>(Response.Status.NOT_FOUND, "Note " + noteId + " not found").build();
   }
   ```
   Or to work on a callback as in this [case](https://github.com/apache/zeppelin/pull/4252/files#diff-94da03110f043520a11e2697bd4b6749134c14214ec240b65a61f6b0ed1f0005R66-R77)
   
   ```java
       return notebook.processNote(noteId,
         jobNote -> {
           List<NoteJobInfo> notesJobInfo = new ArrayList<>();
           if (jobNote == null) {
             callback.onFailure(new IOException("Note " + noteId + " not found"), context);
           } else {
             notesJobInfo.add(new NoteJobInfo(jobNote));
             callback.onSuccess(notesJobInfo, context);
           }
           return notesJobInfo;
         });
   ```
   
   for logging as in this [case](https://github.com/apache/zeppelin/pull/4252/files#diff-c8d59c9816d0b26045cd0cd44343eb10dbebd61730fadae84a9b147e3b6226f8R1829-R1831)
   
   ```java
         getNotebook().processNote(noteId,
           note -> {
             if (note == null) {
               // It is possible the note is removed, but the job is still running
               LOG.warn("Note {} doesn't existed, it maybe deleted.", noteId);
             } else {
               note.clearParagraphOutput(paragraphId);
               Paragraph paragraph = note.getParagraph(paragraphId);
               broadcastParagraph(note, paragraph, MSG_ID_NOT_DEFINED);
             }
             return null;
           });
   ```




-- 
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: dev-unsubscribe@zeppelin.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org