You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@zeppelin.apache.org by DrIgor <gi...@git.apache.org> on 2017/02/20 08:30:16 UTC
[GitHub] zeppelin pull request #2039: [MINOR] Use standard java API to interrupt thre...
GitHub user DrIgor opened a pull request:
https://github.com/apache/zeppelin/pull/2039
[MINOR] Use standard java API to interrupt thread
### What is this PR for?
Use java `Thread.interrupt` method to stop job progress polling thread.
Standard API is:
* proper synchronized
* able to interrupt `Thread.sleep` with any interval
### What type of PR is it?
[Refactoring]
### Questions:
* Does the licenses files need update? no
* Is there breaking changes for older versions? no
* Does this needs documentation? no
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/DrIgor/zeppelin refactor-progress-poller
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/zeppelin/pull/2039.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #2039
----
commit 895787f244792427b397a970a9cc452c9919307f
Author: Igor Drozdov <ig...@epam.com>
Date: 2017-02-20T07:04:12Z
Use standard java API to interrupt thread
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zeppelin issue #2039: [MINOR] Use standard java API to interrupt thread
Posted by DrIgor <gi...@git.apache.org>.
Github user DrIgor commented on the issue:
https://github.com/apache/zeppelin/pull/2039
@felixcheung `terminate` flag is shared across several threads and is accessed without any synchronization.
And we don't need to implement custom interruption flag because java already has it \u2014 `Thread.interrupt`
Standard flag is even better, because it can interrupt `Thread.sleep`.
This PR doesn't change any logic, it just replaces `terminate` with `Thread.interrupted()` and provides proper handling of `InterruptedException`
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zeppelin issue #2039: [MINOR] Use standard java API to interrupt thread
Posted by felixcheung <gi...@git.apache.org>.
Github user felixcheung commented on the issue:
https://github.com/apache/zeppelin/pull/2039
ok LGTM.
any comment from other reviewers?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zeppelin issue #2039: [MINOR] Use standard java API to interrupt thread
Posted by DrIgor <gi...@git.apache.org>.
Github user DrIgor commented on the issue:
https://github.com/apache/zeppelin/pull/2039
Ping-ping)
Do you have other questions? Do you think it's better to proper synchronize `terminate` flag instead?
I reviewed code to understand how zeppelin core works and found several multi threading issues. I would like to help to fix them so I created this PR
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zeppelin issue #2039: [MINOR] Use standard java API to interrupt thread
Posted by felixcheung <gi...@git.apache.org>.
Github user felixcheung commented on the issue:
https://github.com/apache/zeppelin/pull/2039
for a change like this, it would be useful to open a JIRA and to discuss why we are making this change
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zeppelin issue #2039: [MINOR] Use standard java API to interrupt thread
Posted by felixcheung <gi...@git.apache.org>.
Github user felixcheung commented on the issue:
https://github.com/apache/zeppelin/pull/2039
merging if no more comment
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zeppelin pull request #2039: [MINOR] Use standard java API to interrupt thre...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/zeppelin/pull/2039
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zeppelin issue #2039: [MINOR] Use standard java API to interrupt thread
Posted by felixcheung <gi...@git.apache.org>.
Github user felixcheung commented on the issue:
https://github.com/apache/zeppelin/pull/2039
Ok I wasn't aware of the thread.interrupt semantic in JVM.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zeppelin issue #2039: [MINOR] Use standard java API to interrupt thread
Posted by Leemoonsoo <gi...@git.apache.org>.
Github user Leemoonsoo commented on the issue:
https://github.com/apache/zeppelin/pull/2039
Code change Looks good to me.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zeppelin issue #2039: [MINOR] Use standard java API to interrupt thread
Posted by DrIgor <gi...@git.apache.org>.
Github user DrIgor commented on the issue:
https://github.com/apache/zeppelin/pull/2039
`interrupt` just sets `Thread.isInterrupted` flag and does not immediately stop the thread. Then thread can check this flag and decide if it needs to stop. So in fact, termination logic remains the same.
Maybe you mixed up `Thread.interrupt` and `Thread.stop` methods? `Thread.stop` terminates a thread without graceful shutdown, `Thread.interrupt` doesn't.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zeppelin issue #2039: [MINOR] Use standard java API to interrupt thread
Posted by felixcheung <gi...@git.apache.org>.
Github user felixcheung commented on the issue:
https://github.com/apache/zeppelin/pull/2039
From my understanding, setting a flag to let the thread gracefully runs to end, seems safer than calling `interrupt` on it
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---