You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@zeppelin.apache.org by GitBox <gi...@apache.org> on 2021/06/14 07:24:21 UTC
[GitHub] [zeppelin] EricGao888 opened a new pull request #4134: [ZEPPELIN-5398] fix ZEPPELIN-5398, make corrupted notes deletable
EricGao888 opened a new pull request #4134:
URL: https://github.com/apache/zeppelin/pull/4134
### What is this PR for?
* fix ZEPPELIN-5398, make corrupted notes deletable
### What type of PR is it?
* Bug Fix
### Todos
* None
### What is the Jira issue?
* https://issues.apache.org/jira/browse/ZEPPELIN-5398
### How should this be tested?
1. Create a new note.
2. Corrupt it in the backend (e.g. mess up the json format).
3. Restart zeppelin server.
4. Delete the corrupted note and refresh page.
5. The corrupted note will be gone.
### Screenshots (if appropriate)
* None
### Questions:
* None
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] zjffdu commented on a change in pull request #4134: [ZEPPELIN-5398] fix ZEPPELIN-5398, make corrupted notes deletable
Posted by GitBox <gi...@apache.org>.
zjffdu commented on a change in pull request #4134:
URL: https://github.com/apache/zeppelin/pull/4134#discussion_r666788283
##########
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) {
+ e.printStackTrace();
+ }
+ notebookService.moveNoteToTrash(corruptedNote.getId(), context, callback);
+ reset(callback);
+ notesInfo = notebookService.listNotesInfo(false, context, callback);
+ assertEquals(3, notesInfo.size());
+ verify(callback).onSuccess(notesInfo, context);
+ notebookService.removeNote(corruptedNote.getId(), context, callback);
Review comment:
Add test of removing note from trash
--
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
[GitHub] [zeppelin] EricGao888 commented on a change in pull request #4134: [ZEPPELIN-5398] fix ZEPPELIN-5398, make corrupted notes deletable
Posted by GitBox <gi...@apache.org>.
EricGao888 commented on a change in pull request #4134:
URL: https://github.com/apache/zeppelin/pull/4134#discussion_r662074295
##########
File path: zeppelin-server/src/test/java/org/apache/zeppelin/service/NotebookServiceTest.java
##########
@@ -263,6 +264,26 @@ 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;;;");
+ myWriter.close();
+ System.out.println("Successfully wrote to the file.");
Review comment:
Done
--
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
[GitHub] [zeppelin] EricGao888 commented on a change in pull request #4134: [ZEPPELIN-5398] fix ZEPPELIN-5398, make corrupted notes deletable
Posted by GitBox <gi...@apache.org>.
EricGao888 commented on a change in pull request #4134:
URL: https://github.com/apache/zeppelin/pull/4134#discussion_r663701441
##########
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 message, Exception e) {
Review comment:
Seems need to reconstruct the method `fromJson` to pass the parameter `noteId`, or maybe I can create another constructor for `CorruptedNoteException` with one more parameter `noteId` and add the noteId into it after catching the exception without noteId?
--
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
[GitHub] [zeppelin] EricGao888 commented on a change in pull request #4134: [ZEPPELIN-5398] fix ZEPPELIN-5398, make corrupted notes deletable
Posted by GitBox <gi...@apache.org>.
EricGao888 commented on a change in pull request #4134:
URL: https://github.com/apache/zeppelin/pull/4134#discussion_r664268798
##########
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 message, Exception e) {
Review comment:
noteId added
--
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
[GitHub] [zeppelin] zjffdu commented on a change in pull request #4134: [ZEPPELIN-5398] fix ZEPPELIN-5398, make corrupted notes deletable
Posted by GitBox <gi...@apache.org>.
zjffdu commented on a change in pull request #4134:
URL: https://github.com/apache/zeppelin/pull/4134#discussion_r666787809
##########
File path: zeppelin-server/src/main/java/org/apache/zeppelin/service/NotebookService.java
##########
@@ -1007,20 +1017,29 @@ public void updatePersonalizedMode(String noteId,
public void moveNoteToTrash(String noteId,
ServiceContext context,
ServiceCallback<Note> callback) throws IOException {
- Note note = notebook.getNote(noteId);
- if (note == null) {
- callback.onFailure(new NoteNotFoundException(noteId), context);
+ if (!checkPermission(noteId, Permission.OWNER, Message.OP.MOVE_NOTE_TO_TRASH, context, callback)) {
return;
}
- if (!checkPermission(noteId, Permission.OWNER, Message.OP.MOVE_NOTE_TO_TRASH, context,
- callback)) {
- return;
- }
- String destNotePath = "/" + NoteManager.TRASH_FOLDER + note.getPath();
+ String destNotePath = "/" + NoteManager.TRASH_FOLDER + notebook.getNoteManager().getNotesInfo().get(noteId);
Review comment:
Why changing `note.getPath()`to `notebook.getNoteManager().getNotesInfo().get(noteId)`
--
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
[GitHub] [zeppelin] zjffdu commented on a change in pull request #4134: [ZEPPELIN-5398] fix ZEPPELIN-5398, make corrupted notes deletable
Posted by GitBox <gi...@apache.org>.
zjffdu commented on a change in pull request #4134:
URL: https://github.com/apache/zeppelin/pull/4134#discussion_r662702134
##########
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 message, Exception e) {
Review comment:
Better to add noteId in constructor
--
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
[GitHub] [zeppelin] EricGao888 removed a comment on pull request #4134: [ZEPPELIN-5398] fix ZEPPELIN-5398, make corrupted notes deletable
Posted by GitBox <gi...@apache.org>.
EricGao888 removed a comment on pull request #4134:
URL: https://github.com/apache/zeppelin/pull/4134#issuecomment-876298745
> @EricGao888 Could you rebase your PR to trigger the CI again ?
Done, thx
--
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
[GitHub] [zeppelin] EricGao888 commented on a change in pull request #4134: [ZEPPELIN-5398] fix ZEPPELIN-5398, make corrupted notes deletable
Posted by GitBox <gi...@apache.org>.
EricGao888 commented on a change in pull request #4134:
URL: https://github.com/apache/zeppelin/pull/4134#discussion_r669327349
##########
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:
It didn't fail, which was kind of wired. Anyway I've fixed it. THX for the comment.
##########
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:
Done
##########
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:
Done, thx for the comment
--
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
[GitHub] [zeppelin] zjffdu commented on pull request #4134: [ZEPPELIN-5398] fix ZEPPELIN-5398, make corrupted notes deletable
Posted by GitBox <gi...@apache.org>.
zjffdu commented on pull request #4134:
URL: https://github.com/apache/zeppelin/pull/4134#issuecomment-878141005
LGTM
--
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
[GitHub] [zeppelin] zjffdu commented on a change in pull request #4134: [ZEPPELIN-5398] fix ZEPPELIN-5398, make corrupted notes deletable
Posted by GitBox <gi...@apache.org>.
zjffdu commented on a change in pull request #4134:
URL: https://github.com/apache/zeppelin/pull/4134#discussion_r651858033
##########
File path: zeppelin-server/src/main/java/org/apache/zeppelin/service/NotebookService.java
##########
@@ -1012,7 +1012,28 @@ public void updatePersonalizedMode(String noteId,
public void moveNoteToTrash(String noteId,
Review comment:
Besides moving to trash, can corrupted note be deleted from trash ?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] Reamer commented on a change in pull request #4134: [ZEPPELIN-5398] fix ZEPPELIN-5398, make corrupted notes deletable
Posted by GitBox <gi...@apache.org>.
Reamer commented on a change in pull request #4134:
URL: https://github.com/apache/zeppelin/pull/4134#discussion_r669342850
##########
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 change to
```
String.format("noteId: %s - %s", noteId, message)
```
Additional string chaining is not necessary
--
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
[GitHub] [zeppelin] zjffdu commented on a change in pull request #4134: [ZEPPELIN-5398] fix ZEPPELIN-5398, make corrupted notes deletable
Posted by GitBox <gi...@apache.org>.
zjffdu commented on a change in pull request #4134:
URL: https://github.com/apache/zeppelin/pull/4134#discussion_r666787809
##########
File path: zeppelin-server/src/main/java/org/apache/zeppelin/service/NotebookService.java
##########
@@ -1007,20 +1017,29 @@ public void updatePersonalizedMode(String noteId,
public void moveNoteToTrash(String noteId,
ServiceContext context,
ServiceCallback<Note> callback) throws IOException {
- Note note = notebook.getNote(noteId);
- if (note == null) {
- callback.onFailure(new NoteNotFoundException(noteId), context);
+ if (!checkPermission(noteId, Permission.OWNER, Message.OP.MOVE_NOTE_TO_TRASH, context, callback)) {
return;
}
- if (!checkPermission(noteId, Permission.OWNER, Message.OP.MOVE_NOTE_TO_TRASH, context,
- callback)) {
- return;
- }
- String destNotePath = "/" + NoteManager.TRASH_FOLDER + note.getPath();
+ String destNotePath = "/" + NoteManager.TRASH_FOLDER + notebook.getNoteManager().getNotesInfo().get(noteId);
Review comment:
Why changing `note.getPath()`to `notebook.getNoteManager().getNotesInfo().get(noteId)`
--
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
[GitHub] [zeppelin] EricGao888 commented on a change in pull request #4134: [ZEPPELIN-5398] fix ZEPPELIN-5398, make corrupted notes deletable
Posted by GitBox <gi...@apache.org>.
EricGao888 commented on a change in pull request #4134:
URL: https://github.com/apache/zeppelin/pull/4134#discussion_r662074614
##########
File path: zeppelin-server/src/test/java/org/apache/zeppelin/service/NotebookServiceTest.java
##########
@@ -263,6 +264,26 @@ 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;;;");
+ myWriter.close();
+ System.out.println("Successfully wrote to the file.");
Review comment:
Done, thx for the suggestions
--
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
[GitHub] [zeppelin] EricGao888 commented on pull request #4134: [ZEPPELIN-5398] fix ZEPPELIN-5398, make corrupted notes deletable
Posted by GitBox <gi...@apache.org>.
EricGao888 commented on pull request #4134:
URL: https://github.com/apache/zeppelin/pull/4134#issuecomment-876298745
> @EricGao888 Could you rebase your PR to trigger the CI again ?
Done, thx
--
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
[GitHub] [zeppelin] zjffdu commented on a change in pull request #4134: [ZEPPELIN-5398] fix ZEPPELIN-5398, make corrupted notes deletable
Posted by GitBox <gi...@apache.org>.
zjffdu commented on a change in pull request #4134:
URL: https://github.com/apache/zeppelin/pull/4134#discussion_r662108616
##########
File path: zeppelin-zengine/src/main/java/org/apache/zeppelin/notebook/Notebook.java
##########
@@ -346,6 +346,15 @@ public void removeNote(Note note, AuthenticationInfo subject) throws IOException
fireNoteRemoveEvent(note, subject);
}
+ public void removeCorruptedNote(String noteId, AuthenticationInfo subject) throws IOException {
+ LOGGER.info("Remove corrupted note: {}", noteId);
+ // Set Remove to true to cancel saving this note
+// note.setRemoved(true);
Review comment:
Indention
--
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
[GitHub] [zeppelin] zjffdu commented on pull request #4134: [ZEPPELIN-5398] fix ZEPPELIN-5398, make corrupted notes deletable
Posted by GitBox <gi...@apache.org>.
zjffdu commented on pull request #4134:
URL: https://github.com/apache/zeppelin/pull/4134#issuecomment-875394959
@EricGao888 Could you rebase your PR to trigger the CI again ?
--
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
[GitHub] [zeppelin] EricGao888 commented on pull request #4134: [ZEPPELIN-5398] fix ZEPPELIN-5398, make corrupted notes deletable
Posted by GitBox <gi...@apache.org>.
EricGao888 commented on pull request #4134:
URL: https://github.com/apache/zeppelin/pull/4134#issuecomment-861331556
> It would be nice to add a unit test about it.
unit test added : )
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] zjffdu commented on a change in pull request #4134: [ZEPPELIN-5398] fix ZEPPELIN-5398, make corrupted notes deletable
Posted by GitBox <gi...@apache.org>.
zjffdu commented on a change in pull request #4134:
URL: https://github.com/apache/zeppelin/pull/4134#discussion_r651857706
##########
File path: zeppelin-server/src/main/java/org/apache/zeppelin/service/NotebookService.java
##########
@@ -1012,7 +1012,28 @@ public void updatePersonalizedMode(String noteId,
public void moveNoteToTrash(String noteId,
ServiceContext context,
ServiceCallback<Note> callback) throws IOException {
- Note note = notebook.getNote(noteId);
+ Note note = null;
+ try {
+ note = notebook.getNote(noteId);
+ } catch (Exception e) {
+ if ((e.getMessage() != null) && (e.getMessage().contains("Fail to parse note json"))) {
Review comment:
It is better to use specific kind of Exception to represent the corrupted note case.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] EricGao888 commented on pull request #4134: [ZEPPELIN-5398] fix ZEPPELIN-5398, make corrupted notes deletable
Posted by GitBox <gi...@apache.org>.
EricGao888 commented on pull request #4134:
URL: https://github.com/apache/zeppelin/pull/4134#issuecomment-859420632
> It would be nice to add a unit test about it.
Sure, will do.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] EricGao888 commented on a change in pull request #4134: [ZEPPELIN-5398] fix ZEPPELIN-5398, make corrupted notes deletable
Posted by GitBox <gi...@apache.org>.
EricGao888 commented on a change in pull request #4134:
URL: https://github.com/apache/zeppelin/pull/4134#discussion_r669326984
##########
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:
Done
--
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
[GitHub] [zeppelin] EricGao888 commented on a change in pull request #4134: [ZEPPELIN-5398] fix ZEPPELIN-5398, make corrupted notes deletable
Posted by GitBox <gi...@apache.org>.
EricGao888 commented on a change in pull request #4134:
URL: https://github.com/apache/zeppelin/pull/4134#discussion_r664268798
##########
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 message, Exception e) {
Review comment:
noteId added
--
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
[GitHub] [zeppelin] asfgit closed pull request #4134: [ZEPPELIN-5398] fix ZEPPELIN-5398, make corrupted notes deletable
Posted by GitBox <gi...@apache.org>.
asfgit closed pull request #4134:
URL: https://github.com/apache/zeppelin/pull/4134
--
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
[GitHub] [zeppelin] EricGao888 commented on a change in pull request #4134: [ZEPPELIN-5398] fix ZEPPELIN-5398, make corrupted notes deletable
Posted by GitBox <gi...@apache.org>.
EricGao888 commented on a change in pull request #4134:
URL: https://github.com/apache/zeppelin/pull/4134#discussion_r669369278
##########
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:
Done, looks much better now. Thx for the suggestions.
--
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
[GitHub] [zeppelin] Reamer commented on a change in pull request #4134: [ZEPPELIN-5398] fix ZEPPELIN-5398, make corrupted notes deletable
Posted by GitBox <gi...@apache.org>.
Reamer commented on a change in pull request #4134:
URL: https://github.com/apache/zeppelin/pull/4134#discussion_r651845240
##########
File path: zeppelin-server/src/test/java/org/apache/zeppelin/service/NotebookServiceTest.java
##########
@@ -263,6 +264,26 @@ 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;;;");
+ myWriter.close();
+ System.out.println("Successfully wrote to the file.");
Review comment:
Please remove `System.out.println` and work with asserts
You should use [try-with-ressource](https://docs.oracle.com/javase/tutorial/essential/exceptions/tryResourceClose.html) for the FileWriter object.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] EricGao888 commented on a change in pull request #4134: [ZEPPELIN-5398] fix ZEPPELIN-5398, make corrupted notes deletable
Posted by GitBox <gi...@apache.org>.
EricGao888 commented on a change in pull request #4134:
URL: https://github.com/apache/zeppelin/pull/4134#discussion_r667659954
##########
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) {
+ e.printStackTrace();
+ }
+ notebookService.moveNoteToTrash(corruptedNote.getId(), context, callback);
+ reset(callback);
+ notesInfo = notebookService.listNotesInfo(false, context, callback);
+ assertEquals(3, notesInfo.size());
+ verify(callback).onSuccess(notesInfo, context);
+ notebookService.removeNote(corruptedNote.getId(), context, callback);
Review comment:
Test of removing note from trash has been added into zeppelin-zengine/src/test/java/org/apache/zeppelin/notebook/NotebookTest.java, thx
--
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
[GitHub] [zeppelin] EricGao888 commented on pull request #4134: [ZEPPELIN-5398] fix ZEPPELIN-5398, make corrupted notes deletable
Posted by GitBox <gi...@apache.org>.
EricGao888 commented on pull request #4134:
URL: https://github.com/apache/zeppelin/pull/4134#issuecomment-876298987
> @EricGao888 Could you rebase your PR to trigger the CI again ?
Done, thx
--
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
[GitHub] [zeppelin] Reamer commented on a change in pull request #4134: [ZEPPELIN-5398] fix ZEPPELIN-5398, make corrupted notes deletable
Posted by 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
[GitHub] [zeppelin] EricGao888 commented on a change in pull request #4134: [ZEPPELIN-5398] fix ZEPPELIN-5398, make corrupted notes deletable
Posted by GitBox <gi...@apache.org>.
EricGao888 commented on a change in pull request #4134:
URL: https://github.com/apache/zeppelin/pull/4134#discussion_r662075370
##########
File path: zeppelin-server/src/main/java/org/apache/zeppelin/service/NotebookService.java
##########
@@ -1012,7 +1012,28 @@ public void updatePersonalizedMode(String noteId,
public void moveNoteToTrash(String noteId,
Review comment:
No, but have already fixed in the last commit.
--
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
[GitHub] [zeppelin] EricGao888 commented on a change in pull request #4134: [ZEPPELIN-5398] fix ZEPPELIN-5398, make corrupted notes deletable
Posted by GitBox <gi...@apache.org>.
EricGao888 commented on a change in pull request #4134:
URL: https://github.com/apache/zeppelin/pull/4134#discussion_r662074971
##########
File path: zeppelin-server/src/main/java/org/apache/zeppelin/service/NotebookService.java
##########
@@ -1012,7 +1012,28 @@ public void updatePersonalizedMode(String noteId,
public void moveNoteToTrash(String noteId,
ServiceContext context,
ServiceCallback<Note> callback) throws IOException {
- Note note = notebook.getNote(noteId);
+ Note note = null;
+ try {
+ note = notebook.getNote(noteId);
+ } catch (Exception e) {
+ if ((e.getMessage() != null) && (e.getMessage().contains("Fail to parse note json"))) {
Review comment:
Improved, thx for the suggestions.
--
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