[GitHub] [zeppelin] Reamer commented on a change in pull request #4263: [ZEPPELIN-5574] ConcurrentModificationException in JsonResponse.toString() when getNote with restAPI

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

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



##########
File path: zeppelin-zengine/src/main/java/org/apache/zeppelin/notebook/Paragraph.java
##########
@@ -78,12 +81,18 @@
   private Date dateUpdated;
   private int progress;
   // paragraph configs like isOpen, colWidth, etc
-  private Map<String, Object> config = new HashMap<>();
+  // Use ConcurrentHashMap to make Note thread-safe which is required by Note serialization
+  // (saving note to NotebookRepo or broadcast to frontend), see ZEPPELIN-5530.
+  private Map<String, Object> config = new ConcurrentHashMap<>();
   // form and parameter settings
   public GUI settings = new GUI();
   private InterpreterResult results;
   // Application states in this paragraph
-  private final List<ApplicationState> apps = new LinkedList<>();
+  private final Queue<ApplicationState> apps = new ConcurrentLinkedQueue<>();

Review comment:
       I see several lines with `synchronized (apps)` in `paragraph.java`. Are they needed after `apps` has been made thread-safe?

##########
File path: zeppelin-zengine/src/main/java/org/apache/zeppelin/notebook/Notebook.java
##########
@@ -327,10 +327,9 @@ public String cloneNote(String sourceNoteId, String newNotePath, AuthenticationI
             newNote.setConfig(new HashMap<>(sourceNote.getConfig()));
             newNote.setInfo(new HashMap<>(sourceNote.getInfo()));
             newNote.setDefaultInterpreterGroup(sourceNote.getDefaultInterpreterGroup());
-            newNote.setNoteForms(new HashMap<>(sourceNote.getNoteForms()));
-            newNote.setNoteParams(new HashMap<>(sourceNote.getNoteParams()));

Review comment:
       I like this approach because it makes it clear that we are creating a flat copy of the HashMap. Your new approach does not show this.
   Perhaps we should have `getNoteForms()` and `getNoteParams()` return an unmodifiable HashMap.

##########
File path: zeppelin-zengine/src/main/java/org/apache/zeppelin/notebook/Note.java
##########
@@ -862,6 +881,9 @@ public boolean isTrash() {
   }
 
   public CopyOnWriteArrayList<Paragraph> getParagraphs() {
+    if (this.paragraphs == null) {

Review comment:
       After #4252, I see no way that `paragraphs` can be 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