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/10/29 08:29:27 UTC
[GitHub] [zeppelin] zjffdu opened a new pull request #4263: [ZEPPELIN-5574] ConcurrentModificationException in JsonResponse.toString() when getNote with restAPI
zjffdu opened a new pull request #4263:
URL: https://github.com/apache/zeppelin/pull/4263
### What is this PR for?
The root cause of this issue is that when Note is serialized to json, it is being modified by another thread. This PR use thread-safe class for Note/Paragraph.
### What type of PR is it?
[Bug Fix ]
### Todos
* [ ] - Task
### What is the Jira issue?
* https://issues.apache.org/jira/browse/ZEPPELIN-5574
### How should this be tested?
* Tested it for 2 days in customer's environment and no issue happen again. (Before this PR, this issue happens everyday)
### Screenshots (if appropriate)
### Questions:
* Does the licenses files need update? No
* Is there breaking changes for older versions? No
* Does this needs documentation? No
--
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 #4263: [ZEPPELIN-5574] ConcurrentModificationException in JsonResponse.toString() when getNote with restAPI
Posted by GitBox <gi...@apache.org>.
Reamer commented on a change in pull request #4263:
URL: https://github.com/apache/zeppelin/pull/4263#discussion_r793335383
##########
File path: zeppelin-zengine/src/main/java/org/apache/zeppelin/notebook/Note.java
##########
@@ -279,22 +284,36 @@ public void setDefaultInterpreterGroup(String defaultInterpreterGroup) {
this.defaultInterpreterGroup = defaultInterpreterGroup;
}
- public Map<String, Object> getNoteParams() {
- return noteParams;
+ public ConcurrentHashMap<String, Object> getNoteParams() {
+ if (noteParams == null) {
+ return new ConcurrentHashMap<>();
+ }
+ return new ConcurrentHashMap<>(noteParams);
}
public void setNoteParams(Map<String, Object> noteParams) {
this.noteParams = noteParams;
}
- public Map<String, Input> getNoteForms() {
- return noteForms;
+ public void removeNoteParam(String key) {
+ noteParams.remove(key);
+ }
+
+ public ConcurrentHashMap<String, Input> getNoteForms() {
+ if (noteForms == null) {
+ return new ConcurrentHashMap<>();
+ }
+ return new ConcurrentHashMap<>(noteForms);
Review comment:
Why return a new ConcurrentHashMap?
I don't know if returning an [Collections.unmodifiableMap](https://docs.oracle.com/javase/7/docs/api/java/util/Collections.html#unmodifiableMap(java.util.Map)) might be a good approach here.
This [code part ](https://github.com/apache/zeppelin/blob/master/zeppelin-zengine/src/main/java/org/apache/zeppelin/notebook/Paragraph.java#L450-L455) should be the only place where the map is edited from another class. Here we could force the class to create a new map to work on and then communicate it to the Note via setNoteForms(..).
--
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 pull request #4263: [ZEPPELIN-5574] ConcurrentModificationException in JsonResponse.toString() when getNote with restAPI
Posted by GitBox <gi...@apache.org>.
Reamer commented on pull request #4263:
URL: https://github.com/apache/zeppelin/pull/4263#issuecomment-1018283283
I merged #4252 into master. Please rebase to the current master. I think this PR is still needed.
--
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 pull request #4263: [ZEPPELIN-5574] ConcurrentModificationException in JsonResponse.toString() when getNote with restAPI
Posted by GitBox <gi...@apache.org>.
Reamer commented on pull request #4263:
URL: https://github.com/apache/zeppelin/pull/4263#issuecomment-961689965
@zjffdu
I want to try to solve the concurrent problem in pull request #4252 with a ReentrantReadWriteLock. This solution should also catch this pull request.
Give me some time for the implementation.
--
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 #4263: [ZEPPELIN-5574] ConcurrentModificationException in JsonResponse.toString() when getNote with restAPI
Posted by GitBox <gi...@apache.org>.
zjffdu commented on pull request #4263:
URL: https://github.com/apache/zeppelin/pull/4263#issuecomment-961747430
@Reamer Sure, let me know whey you are ready
--
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 #4263: [ZEPPELIN-5574] ConcurrentModificationException in JsonResponse.toString() when getNote with restAPI
Posted by GitBox <gi...@apache.org>.
Reamer commented on a change in pull request #4263:
URL: https://github.com/apache/zeppelin/pull/4263#discussion_r793335383
##########
File path: zeppelin-zengine/src/main/java/org/apache/zeppelin/notebook/Note.java
##########
@@ -279,22 +284,36 @@ public void setDefaultInterpreterGroup(String defaultInterpreterGroup) {
this.defaultInterpreterGroup = defaultInterpreterGroup;
}
- public Map<String, Object> getNoteParams() {
- return noteParams;
+ public ConcurrentHashMap<String, Object> getNoteParams() {
+ if (noteParams == null) {
+ return new ConcurrentHashMap<>();
+ }
+ return new ConcurrentHashMap<>(noteParams);
}
public void setNoteParams(Map<String, Object> noteParams) {
this.noteParams = noteParams;
}
- public Map<String, Input> getNoteForms() {
- return noteForms;
+ public void removeNoteParam(String key) {
+ noteParams.remove(key);
+ }
+
+ public ConcurrentHashMap<String, Input> getNoteForms() {
+ if (noteForms == null) {
+ return new ConcurrentHashMap<>();
+ }
+ return new ConcurrentHashMap<>(noteForms);
Review comment:
Why return a new ConcurrentHashMap?
I don't know if returning an [Collections.unmodifiableMap](https://docs.oracle.com/javase/7/docs/api/java/util/Collections.html#unmodifiableMap(java.util.Map)) might be a good approach here.
This [code part ](https://github.com/apache/zeppelin/blob/master/zeppelin-zengine/src/main/java/org/apache/zeppelin/notebook/Paragraph.java#L450-L455) should be the only place where the map is edited from another class. Here qw could force the class to create a new map to work on and then communicate it to the Note.
--
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 #4263: [ZEPPELIN-5574] ConcurrentModificationException in JsonResponse.toString() when getNote with restAPI
Posted by GitBox <gi...@apache.org>.
zjffdu commented on a change in pull request #4263:
URL: https://github.com/apache/zeppelin/pull/4263#discussion_r793236551
##########
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:
Good catch, I don't think it is necessary, I have removed it.
--
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 pull request #4263: [ZEPPELIN-5574] ConcurrentModificationException in JsonResponse.toString() when getNote with restAPI
Posted by GitBox <gi...@apache.org>.
Reamer commented on pull request #4263:
URL: https://github.com/apache/zeppelin/pull/4263#issuecomment-961689965
@zjffdu
I want to try to solve the concurrent problem in pull request #4252 with a ReentrantReadWriteLock. This solution should also catch this pull request.
Give me some time for the implementation.
--
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 #4263: [ZEPPELIN-5574] ConcurrentModificationException in JsonResponse.toString() when getNote with restAPI
Posted by GitBox <gi...@apache.org>.
zjffdu commented on pull request #4263:
URL: https://github.com/apache/zeppelin/pull/4263#issuecomment-961747430
@Reamer Sure, let me know whey you are ready
--
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 #4263: [ZEPPELIN-5574] ConcurrentModificationException in JsonResponse.toString() when getNote with restAPI
Posted by GitBox <gi...@apache.org>.
zjffdu commented on pull request #4263:
URL: https://github.com/apache/zeppelin/pull/4263#issuecomment-961747430
@Reamer Sure, let me know whey you are ready
--
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 #4263: [ZEPPELIN-5574] ConcurrentModificationException in JsonResponse.toString() when getNote with restAPI
Posted by GitBox <gi...@apache.org>.
Reamer commented on a change in pull request #4263:
URL: https://github.com/apache/zeppelin/pull/4263#discussion_r793335383
##########
File path: zeppelin-zengine/src/main/java/org/apache/zeppelin/notebook/Note.java
##########
@@ -279,22 +284,36 @@ public void setDefaultInterpreterGroup(String defaultInterpreterGroup) {
this.defaultInterpreterGroup = defaultInterpreterGroup;
}
- public Map<String, Object> getNoteParams() {
- return noteParams;
+ public ConcurrentHashMap<String, Object> getNoteParams() {
+ if (noteParams == null) {
+ return new ConcurrentHashMap<>();
+ }
+ return new ConcurrentHashMap<>(noteParams);
}
public void setNoteParams(Map<String, Object> noteParams) {
this.noteParams = noteParams;
}
- public Map<String, Input> getNoteForms() {
- return noteForms;
+ public void removeNoteParam(String key) {
+ noteParams.remove(key);
+ }
+
+ public ConcurrentHashMap<String, Input> getNoteForms() {
+ if (noteForms == null) {
+ return new ConcurrentHashMap<>();
+ }
+ return new ConcurrentHashMap<>(noteForms);
Review comment:
Why return a new ConcurrentHashMap?
I don't know if returning an Collections.unmodifiableMap might be a good approach here.
This [code part ](https://github.com/apache/zeppelin/blob/master/zeppelin-zengine/src/main/java/org/apache/zeppelin/notebook/Paragraph.java#L450-L455) should be the only place where the map is edited from another class. Here qw could force the class to create a new map to work on and then communicate it to the Note.
--
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 #4263: [ZEPPELIN-5574] ConcurrentModificationException in JsonResponse.toString() when getNote with restAPI
Posted by 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
[GitHub] [zeppelin] Reamer commented on a change in pull request #4263: [ZEPPELIN-5574] ConcurrentModificationException in JsonResponse.toString() when getNote with restAPI
Posted by GitBox <gi...@apache.org>.
Reamer commented on a change in pull request #4263:
URL: https://github.com/apache/zeppelin/pull/4263#discussion_r793335077
##########
File path: zeppelin-zengine/src/main/java/org/apache/zeppelin/notebook/Note.java
##########
@@ -279,22 +284,36 @@ public void setDefaultInterpreterGroup(String defaultInterpreterGroup) {
this.defaultInterpreterGroup = defaultInterpreterGroup;
}
- public Map<String, Object> getNoteParams() {
- return noteParams;
+ public ConcurrentHashMap<String, Object> getNoteParams() {
+ if (noteParams == null) {
+ return new ConcurrentHashMap<>();
+ }
+ return new ConcurrentHashMap<>(noteParams);
Review comment:
Why return a new ConcurrentHashMap?
I don't know if returning an Collections.unmodifiableMap might be a good approach here.
This [code part ](https://github.com/apache/zeppelin/blob/master/zeppelin-zengine/src/main/java/org/apache/zeppelin/notebook/Paragraph.java#L450-L455) should be the only place where the map is edited from another class. Here qw could force the class to create a new map to work on and then communicate it to the Note.
--
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 pull request #4263: [ZEPPELIN-5574] ConcurrentModificationException in JsonResponse.toString() when getNote with restAPI
Posted by GitBox <gi...@apache.org>.
Reamer commented on pull request #4263:
URL: https://github.com/apache/zeppelin/pull/4263#issuecomment-961689965
@zjffdu
I want to try to solve the concurrent problem in pull request #4252 with a ReentrantReadWriteLock. This solution should also catch this pull request.
Give me some time for the implementation.
--
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 #4263: [ZEPPELIN-5574] ConcurrentModificationException in JsonResponse.toString() when getNote with restAPI
Posted by GitBox <gi...@apache.org>.
zjffdu commented on a change in pull request #4263:
URL: https://github.com/apache/zeppelin/pull/4263#discussion_r793235588
##########
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:
You are right, I have reverted this change
--
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