[GitHub] [zeppelin] Reamer commented on a change in pull request #4134: [ZEPPELIN-5398] fix ZEPPELIN-5398, make corrupted notes deletable

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

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



##########
File path: zeppelin-server/src/test/java/org/apache/zeppelin/service/NotebookServiceTest.java
##########
@@ -263,6 +264,22 @@ public void testNoteOperations() throws IOException {
     assertEquals(2, notesInfo.size());
     verify(callback).onSuccess(notesInfo, context);
 
+    // test moving corrupted note to trash
+    Note corruptedNote = notebookService.createNote("/folder_1/corruptedNote", "test", true, context, callback);
+    String corruptedNotePath = notebookDir.getAbsolutePath() + corruptedNote.getPath() + "_" + corruptedNote.getId() + ".zpln";
+    // corrupt note
+    try (FileWriter myWriter = new FileWriter(corruptedNotePath)) {
+      myWriter.write("{{{I'm corrupted;;;");
+    } catch (IOException e) {

Review comment:
       The `IOException` does not need to be caught because the declaration of the test method allows `IOException` to be thrown.

##########
File path: zeppelin-zengine/src/main/java/org/apache/zeppelin/notebook/exception/CorruptedNoteException.java
##########
@@ -0,0 +1,26 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *    http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.zeppelin.notebook.exception;
+
+import java.io.IOException;
+
+public class CorruptedNoteException extends IOException {
+    public CorruptedNoteException(final String noteId, final String message, Exception e) {
+        super(String.format("noteId: %s", noteId) + message, e);

Review comment:
       Please use a delimiter between NodeID and message.
   Output should be something like: `nodeID: 1234 - Fail to parse note json ...`

##########
File path: zeppelin-zengine/src/test/java/org/apache/zeppelin/notebook/NotebookTest.java
##########
@@ -470,6 +471,29 @@ public void testRemoveNote() throws IOException, InterruptedException {
     }
   }
 
+  @Test
+  public void testRemoveCorruptedNote() throws IOException, InterruptedException {
+    try {
+      LOGGER.info("--------------- Test testRemoveCorruptedNote ---------------");
+      // create a note and a paragraph
+      Note corruptedNote = notebook.createNote("note1", anonymous);
+      String corruptedNotePath = notebookDir.getAbsolutePath() + corruptedNote.getPath() + "_" + corruptedNote.getId() + ".zpln";
+      // corrupt note
+      try (FileWriter myWriter = new FileWriter(corruptedNotePath)) {
+        myWriter.write("{{{I'm corrupted;;;");
+      } catch (IOException e) {

Review comment:
       Same, no need to catch the `IOException`.

##########
File path: zeppelin-zengine/src/test/java/org/apache/zeppelin/notebook/NotebookTest.java
##########
@@ -470,6 +471,29 @@ public void testRemoveNote() throws IOException, InterruptedException {
     }
   }
 
+  @Test
+  public void testRemoveCorruptedNote() throws IOException, InterruptedException {
+    try {
+      LOGGER.info("--------------- Test testRemoveCorruptedNote ---------------");
+      // create a note and a paragraph
+      Note corruptedNote = notebook.createNote("note1", anonymous);
+      String corruptedNotePath = notebookDir.getAbsolutePath() + corruptedNote.getPath() + "_" + corruptedNote.getId() + ".zpln";
+      // corrupt note
+      try (FileWriter myWriter = new FileWriter(corruptedNotePath)) {
+        myWriter.write("{{{I'm corrupted;;;");
+      } catch (IOException e) {
+        e.printStackTrace();
+      }
+      LOGGER.info("--------------- Finish Test testRemoveCorruptedNote ---------------");
+      int numberOfNotes = notebook.getAllNotes().size();
+      notebook.removeNote(corruptedNote, anonymous);
+      assertEquals(numberOfNotes - 1, notebook.getAllNotes().size());
+      LOGGER.info("--------------- Finish Test testRemoveCorruptedNote ---------------");
+    } catch (Exception e) {
+      e.printStackTrace();

Review comment:
       I think your test should fail at this point, right?
   If you catch all exceptions, then you should remove `throws IOException, InterruptedException` from your test method declaration.




-- 
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