[GitHub] [submarine] pingsutw commented on a change in pull request #689: SUBMARINE-911. Fix the bug about deleting notebook

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

pingsutw commented on a change in pull request #689:
URL: https://github.com/apache/submarine/pull/689#discussion_r683152389



##########
File path: submarine-server/server-core/src/main/java/org/apache/submarine/server/notebook/NotebookManager.java
##########
@@ -118,79 +118,89 @@ public Notebook createNotebook(NotebookSpec spec) throws SubmarineRuntimeExcepti
 
   /**
    * Get a list of notebook with user id
+   *
    * @param id user id
    * @return a list of notebook
    */
   public List<Notebook> listNotebooksByUserId(String id) {
-    List<Notebook> notebookList = submitter.listNotebook(id);
-    for (Notebook notebook : notebookList) {
-      for (Map.Entry<String, Notebook> entry : cachedNotebookMap.entrySet()) {
-        Notebook cachedNotebook = entry.getValue();
-        if (cachedNotebook.getUid().equals(notebook.getUid())) {
-          notebook.setNotebookId(cachedNotebook.getNotebookId());
-          notebook.setSpec(cachedNotebook.getSpec());
-        }
+    List<Notebook> serviceNotebooks = notebookService.selectAll();
+    List<Notebook> notebookList = new ArrayList<>();
+    for (Notebook nb : serviceNotebooks) {
+      try {
+        Notebook notebook = submitter.findNotebook(nb.getSpec());
+        notebook.setNotebookId(nb.getNotebookId());
+        notebook.setSpec(nb.getSpec());
+        notebookList.add(notebook);
+      } catch (SubmarineRuntimeException e) {
+        LOG.warn("Submitter can not find notebook: {}, will delete it", nb.getNotebookId());
+        notebookService.delete(nb.getNotebookId().toString());
+        continue;
       }
     }
     return notebookList;
   }
 
   /**
    * Get a notebook instance
+   *
    * @param id notebook id
    * @return object
    * @throws SubmarineRuntimeException the service error
    */
   public Notebook getNotebook(String id) throws SubmarineRuntimeException {
     checkNotebookId(id);
-    Notebook notebook = cachedNotebookMap.get(id);
-    NotebookSpec spec = notebook.getSpec();
-    Notebook patchNotebook = submitter.findNotebook(spec);
-    notebook.rebuild(patchNotebook);
-    return notebook;
+
+    Notebook notebook = notebookService.select(id);
+    Notebook foundNotebook = submitter.findNotebook(notebook.getSpec());
+    foundNotebook.rebuild(notebook);
+
+    return foundNotebook;
   }
 
   /**
    * Delete the notebook instance
+   *
    * @param id notebook id
    * @return object
    * @throws SubmarineRuntimeException the service error
    */
   public Notebook deleteNotebook(String id) throws SubmarineRuntimeException {
-    checkNotebookId(id);
-    Notebook notebook = cachedNotebookMap.remove(id);
-    NotebookSpec spec = notebook.getSpec();
-    Notebook patchNotebook = submitter.deleteNotebook(spec);
+    Notebook notebook = getNotebook(id);
+    Notebook patchNotebook = submitter.deleteNotebook(notebook.getSpec());
+    notebookService.delete(id);
     notebook.rebuild(patchNotebook);
     return notebook;
   }
 
   /**
    * Generate a unique notebook id
+   *
    * @return notebook id
    */
   private NotebookId generateNotebookId() {
     return NotebookId.newInstance(SubmarineServer.getServerTimeStamp(),
-            notebookCounter.incrementAndGet());
+        notebookCounter.incrementAndGet());
   }
 
   /**
    * Check if notebook spec is valid
+   *
    * @param spec notebook spec
    */
   private void checkNotebookSpec(NotebookSpec spec) {
     //TODO(ryan): The method need to be improved
     if (spec == null) {
       throw new SubmarineRuntimeException(Response.Status.OK.getStatusCode(),
-              "Invalid. Notebook Spec object is null.");
+          "Invalid. Notebook Spec object is null.");
     }
   }
 
   private void checkNotebookId(String id) throws SubmarineRuntimeException {
     NotebookId notebookId = NotebookId.fromString(id);
-    if (notebookId == null || !cachedNotebookMap.containsKey(id)) {
+    if (notebookId == null) {

Review comment:
       Don't we need to check if notebookId is in the database? 




-- 
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: commits-unsubscribe@submarine.apache.org

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