Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

ZEPPELIN-3411 Long running logic inside synchronized block in InterpreterSettingManager #2935

Closed
wants to merge 3 commits into from
Closed

ZEPPELIN-3411 Long running logic inside synchronized block in InterpreterSettingManager #2935

wants to merge 3 commits into from

Conversation

jongyoul
Copy link
Member

What is this PR for?

Removing redundant synchronized code to avoid blocking other logics.

What type of PR is it?

[Bug Fix]

Todos

  • - Change synchronized block to read/write lock

What is the Jira issue?

How should this be tested?

  • Current tests should be passed

Screenshots (if appropriate)

Questions:

  • Does the licenses files need update? No
  • Is there breaking changes for older versions? No
  • Does this needs documentation? No

@jongyoul
Copy link
Member Author

Test passed. https://travis-ci.org/jongyoul/zeppelin/builds/367566398

jongyoul
jongyoul commented Apr 22, 2018
View reviewed changes
zeppelin-zengine/src/main/java/org/apache/zeppelin/interpreter/InterpreterSettingManager.java
//clean up metaInfos
intpSetting.setInfos(null);
copyDependenciesFromLocalPath(intpSetting);
intpSetting.closeInterpreters(user, noteId);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@zjffdu It was the original problem.

@jongyoul
Copy link
Member Author

This is not a critial issue when running normally. But in case of problemistic situation like restarting forcibly to stop long running job of SparkInterpreter, some logic hangs inside synchronized block.

@zjffdu
Copy link
Contributor

zjffdu commented Apr 22, 2018 •
edited

Do you have jstack so that I can understand what the exact problem is ?

@jongyoul
Copy link
Member Author

@zjffdu Let me make it

@jongyoul
Copy link
Member Author

image

image

image

@zjffdu I've received this issue report from one of my colleagues that he ran a wrong code to read all of the data from a large directory and it took for 20mins with a spark job and he tried to stop a spark interpreter. and Then he tried to create a new note, but he couldn't see an interpreter lists from creating note page and a list-interpreter when clicking a button on a note.

BUT I've tried to make it with current master into my local but it fails because 'restart' always work fine. It looks only until 0.7.3.

Generally, this code doesn't a problem anymore in current master and branch-0.8, but this kind of synchronized blocks don't look good. I prefer to change it but it's not a bug now. What do you think of it?

zjffdu
zjffdu reviewed Apr 23, 2018
View reviewed changes
zeppelin-zengine/src/main/java/org/apache/zeppelin/interpreter/InterpreterSettingManager.java Outdated
@@ -103,6 +104,7 @@
*/
private final Map<String, InterpreterSetting> interpreterSettings =
Maps.newConcurrentMap();
private final ReentrantReadWriteLock interpreterSettingsLock = new ReentrantReadWriteLock();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually interpreterSettings is ConcurrentHashMap, I think we don't need extra lock here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I thought it and wanted to confirm it. Then, I'll remove all of the locks related to interpreterSettings in this class.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

@jongyoul
Copy link
Member Author

Removed all of the synchronized blocks from InterpreterSettingManager for interpreterSettings

zjffdu
zjffdu reviewed Apr 23, 2018
View reviewed changes
zeppelin-zengine/src/main/java/org/apache/zeppelin/interpreter/InterpreterSettingManager.java Outdated
closeThreads.add(t);
}
Collection<InterpreterSetting> intpSettings = interpreterSettings.values();
for (final InterpreterSetting intpSetting : intpSettings) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Merge them into one line

for (InterpreterSetting intpSetting : interpreterSettings.values()) {

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

zjffdu
zjffdu reviewed Apr 23, 2018
View reviewed changes
zeppelin-zengine/src/main/java/org/apache/zeppelin/interpreter/InterpreterSettingManager.java Outdated
@@ -24,6 +24,7 @@
import com.google.gson.Gson;
import com.google.gson.GsonBuilder;
import com.google.gson.reflect.TypeToken;
import java.util.concurrent.locks.ReentrantReadWriteLock;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this could be removed

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

@zjffdu
Copy link
Contributor

zjffdu commented Apr 23, 2018

THanks @jongyoul Just several minor issues, rest look good to me.

@jongyoul
Removed unused import statements
3b90155 
Simplified `for` loop
@jongyoul
Copy link
Member Author

After finishing CI, I'll merge it

@jongyoul
Copy link
Member Author

Test passed. https://travis-ci.org/jongyoul/zeppelin/builds/369961193 Will merge it

@asfgit asfgit closed this in 75dd195 Apr 23, 2018
weand pushed a commit to weand/zeppelin that referenced this pull request Apr 23, 2018
@jongyoul @weand
ZEPPELIN-3411 Long running logic inside synchronized block in Interpr…
dc05b83 
…eterSettingManager

### What is this PR for?
Removing redundant synchronized code to avoid blocking other logics.

### What type of PR is it?
[Bug Fix]

### Todos
* [x] - Change synchronized block to read/write lock

### What is the Jira issue?
* https://issues.apache.org/jira/browse/ZEPPELIN-3411

### How should this be tested?
* Current tests should be passed

### Screenshots (if appropriate)

### Questions:
* Does the licenses files need update? No
* Is there breaking changes for older versions? No
* Does this needs documentation? No

Author: Jongyoul Lee <jongyoul@gmail.com>

Closes apache#2935 from jongyoul/ZEPPELIN-3411 and squashes the following commits:

3b90155 [Jongyoul Lee] Removed unused `import` statements Simplified `for` loop
4691301 [Jongyoul Lee] Removed lock/synchronized codes because interpreterSettings already is implemented by currentHashMap
24be692 [Jongyoul Lee] Removed all synchronized blocks and replace them to read/write lock
@weand
Copy link
Contributor

weand commented Apr 23, 2018

are you going to merge that into branch-0.8 as well?

@jongyoul
Copy link
Member Author

@weand Oops. I was confused with another issue. I’ll cherry-pick this into 0.8

asfgit pushed a commit that referenced this pull request Apr 24, 2018
@jongyoul
ZEPPELIN-3411 Long running logic inside synchronized block in Interpr…
6559c5e 
…eterSettingManager

### What is this PR for?
Removing redundant synchronized code to avoid blocking other logics.

### What type of PR is it?
[Bug Fix]

### Todos
* [x] - Change synchronized block to read/write lock

### What is the Jira issue?
* https://issues.apache.org/jira/browse/ZEPPELIN-3411

### How should this be tested?
* Current tests should be passed

### Screenshots (if appropriate)

### Questions:
* Does the licenses files need update? No
* Is there breaking changes for older versions? No
* Does this needs documentation? No

Author: Jongyoul Lee <jongyoul@gmail.com>

Closes #2935 from jongyoul/ZEPPELIN-3411 and squashes the following commits:

3b90155 [Jongyoul Lee] Removed unused `import` statements Simplified `for` loop
4691301 [Jongyoul Lee] Removed lock/synchronized codes because interpreterSettings already is implemented by currentHashMap
24be692 [Jongyoul Lee] Removed all synchronized blocks and replace them to read/write lock
prabhjyotsingh pushed a commit to prabhjyotsingh/zeppelin that referenced this pull request Jul 4, 2018
@jongyoul @zjffdu
ZEPPELIN-3411 Long running logic inside synchronized block in Interpr…
f2f2494 
…eterSettingManager

Removing redundant synchronized code to avoid blocking other logics.

[Bug Fix]

* [x] - Change synchronized block to read/write lock

* https://issues.apache.org/jira/browse/ZEPPELIN-3411

* Current tests should be passed

* Does the licenses files need update? No
* Is there breaking changes for older versions? No
* Does this needs documentation? No

Author: Jongyoul Lee <jongyoul@gmail.com>

Closes apache#2935 from jongyoul/ZEPPELIN-3411 and squashes the following commits:

3b90155 [Jongyoul Lee] Removed unused `import` statements Simplified `for` loop
4691301 [Jongyoul Lee] Removed lock/synchronized codes because interpreterSettings already is implemented by currentHashMap
24be692 [Jongyoul Lee] Removed all synchronized blocks and replace them to read/write lock

(cherry picked from commit 6559c5e)

Change-Id: Ic28a3e9cf8ec93c84596861716c3cc25dedd96f5
mckartha pushed a commit to syntechdev/zeppelin that referenced this pull request Aug 9, 2018
@jongyoul @mckartha
ZEPPELIN-3411 Long running logic inside synchronized block in Interpr…
732003d 
…eterSettingManager

### What is this PR for?
Removing redundant synchronized code to avoid blocking other logics.

### What type of PR is it?
[Bug Fix]

### Todos
* [x] - Change synchronized block to read/write lock

### What is the Jira issue?
* https://issues.apache.org/jira/browse/ZEPPELIN-3411

### How should this be tested?
* Current tests should be passed

### Screenshots (if appropriate)

### Questions:
* Does the licenses files need update? No
* Is there breaking changes for older versions? No
* Does this needs documentation? No

Author: Jongyoul Lee <jongyoul@gmail.com>

Closes apache#2935 from jongyoul/ZEPPELIN-3411 and squashes the following commits:

3b90155 [Jongyoul Lee] Removed unused `import` statements Simplified `for` loop
4691301 [Jongyoul Lee] Removed lock/synchronized codes because interpreterSettings already is implemented by currentHashMap
24be692 [Jongyoul Lee] Removed all synchronized blocks and replace them to read/write lock
mckartha pushed a commit to syntechdev/zeppelin that referenced this pull request Aug 9, 2018
@jongyoul @mckartha
ZEPPELIN-3411 Long running logic inside synchronized block in Interpr…
f094b89 
…eterSettingManager

### What is this PR for?
Removing redundant synchronized code to avoid blocking other logics.

### What type of PR is it?
[Bug Fix]

### Todos
* [x] - Change synchronized block to read/write lock

### What is the Jira issue?
* https://issues.apache.org/jira/browse/ZEPPELIN-3411

### How should this be tested?
* Current tests should be passed

### Screenshots (if appropriate)

### Questions:
* Does the licenses files need update? No
* Is there breaking changes for older versions? No
* Does this needs documentation? No

Author: Jongyoul Lee <jongyoul@gmail.com>

Closes apache#2935 from jongyoul/ZEPPELIN-3411 and squashes the following commits:

3b90155 [Jongyoul Lee] Removed unused `import` statements Simplified `for` loop
4691301 [Jongyoul Lee] Removed lock/synchronized codes because interpreterSettings already is implemented by currentHashMap
24be692 [Jongyoul Lee] Removed all synchronized blocks and replace them to read/write lock
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zjffdu zjffdu zjffdu left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@jongyoul @zjffdu @weand