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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
