ZEPPELIN-1420. java.util.ConcurrentModificationException caused by calling remove inside foreach loop #1419
Conversation
…lling remove inside foreach loop
zjffdu
force-pushed
the
ZEPPELIN-1420
branch
from
5431de5 to
ddd0710
Compare
| } | ||
|
|
||
| Iterator<String> iter = interpreterSettingIds.iterator(); | ||
| while (iter.hasNext()) { |
There was a problem hiding this comment.
I also remove the synchronized block, it seems not necessary.
|
LGTM and merge if there're no more discussions. |
| InterpreterSetting setting = get(id); | ||
| if (setting == null) { | ||
| // interpreter setting is removed from factory. remove id from here, too | ||
| iter.remove(); |
There was a problem hiding this comment.
What is the purpose of removing the id? It seems to me that interpreterSettingIds is a temporary LinkedList and the effect of removing id is not persisted.
private List<String> getNoteInterpreterSettingBinding(String noteId) {
LinkedList<String> bindings = new LinkedList<>();
synchronized (interpreterSettings) {
List<String> settingIds = interpreterBindings.get(noteId);
if (settingIds != null) {
bindings.addAll(settingIds);
}
}
return bindings;
}
There was a problem hiding this comment.
@prasadwagle It is because the interpreter setting may be removed in other threads, so we need to check it here. I don't quite understand your concern about the persistence.
There was a problem hiding this comment.
A few weeks ago, I came across the exact same question as @prasadwagle. Call remote on the temporary (not returned) list is useless (this consideration/question is separate from this PR which aims solving the concurrent modification exception (which I met also), but is worth to consider).
There was a problem hiding this comment.
that's good question although not related to topic of this PR. maybe @jongyoul could bring little clarity as I believe he's the original author of that function
|
Shall we merge this guy now? |
|
tested locally, LGTM |
|
Merging to master, if there is no further discussion. |
…lling remove inside foreach loop ### What is this PR for? We should use Iterator to iterate the list when we want to remove items in the middle of iteration, Otherwise will get the ConcurrentModificationException ### What type of PR is it? [Bug Fix] ### Todos * [ ] - Task ### What is the Jira issue? * https://issues.apache.org/jira/browse/ZEPPELIN-1420 ### How should this be tested? No test added ### Questions: * Does the licenses files need update? No * Is there breaking changes for older versions? No * Does this needs documentation? No Author: Jeff Zhang <[email protected]> Closes apache#1419 from zjffdu/ZEPPELIN-1420 and squashes the following commits: ddd0710 [Jeff Zhang] ZEPPELIN-1420. java.util.ConcurrentModificationException caused by calling remove inside foreach loop
6 participants
What is this PR for?
We should use Iterator to iterate the list when we want to remove items in the middle of iteration, Otherwise will get the ConcurrentModificationException
What type of PR is it?
[Bug Fix]
Todos
What is the Jira issue?
How should this be tested?
No test added
Questions: