You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by sarutak <gi...@git.apache.org> on 2014/11/06 23:01:52 UTC
[GitHub] spark pull request: [SPARK-4282] Stopping flag in YarnClientSchedu...
GitHub user sarutak opened a pull request:
https://github.com/apache/spark/pull/3143
[SPARK-4282] Stopping flag in YarnClientSchedulerBackend should be volatile
In YarnClientSchedulerBackend, a variable "stopping" is used as a flag and it's accessed by some threads so it should be volatile.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/sarutak/spark stopping-flag-volatile
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/spark/pull/3143.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 #3143
----
commit 58fdcc9024b97741e8fbaf7768dfc3ecfa9a3792
Author: Kousuke Saruta <sa...@oss.nttdata.co.jp>
Date: 2014-11-06T21:58:28Z
Marked stoppig flag as volatile
----
---
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-4282][YARN] Stopping flag in YarnClient...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/3143#issuecomment-62059571
[Test build #23019 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/23019/consoleFull) for PR 3143 at commit [`58fdcc9`](https://github.com/apache/spark/commit/58fdcc9024b97741e8fbaf7768dfc3ecfa9a3792).
* This patch merges cleanly.
---
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-4282][YARN] Stopping flag in YarnClient...
Posted by sarutak <gi...@git.apache.org>.
Github user sarutak commented on the pull request:
https://github.com/apache/spark/pull/3143#issuecomment-62239300
@JoshRosen You mean we should call `ClientBase#monitorApplication()` with blocking parameter instead of implementing similar code. In a nut shell, like following code?
private def asyncMonitorApplication(): Unit = {
assert(client != null && appId != null, "Application has not been submitted yet!")
val t = new Thread {
override def run() {
val (state, _) = client.monitorApplication(appId, returnOnRunning = false) // blocking
if (!stopping) {
logError(s"Yarn application has already exited with state $state!")
sc.stop()
}
}
}
t.setName("Yarn application state monitor")
t.setDaemon(true)
t.start()
}
---
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-4282][YARN] Stopping flag in YarnClient...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/3143#issuecomment-62070687
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/23019/
Test PASSed.
---
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-4282][YARN] Stopping flag in YarnClient...
Posted by JoshRosen <gi...@git.apache.org>.
Github user JoshRosen commented on the pull request:
https://github.com/apache/spark/pull/3143#issuecomment-62214013
> the current pattern allows someone calling sc.stop() to terminate this thread cleanly, independent of what YARN says
It looks like `stop()` calls `client.stop()`, which could implement the volatile variable to interrupt any threads that called `monitorApplication`. So, it sounds like we'll probably end up implementing the `@volatile` in either case, but I'm suggesting that this synchronization / interruption logic should be hidden inside the `Client` rather than exposed here.
---
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-4282][YARN] Stopping flag in YarnClient...
Posted by aarondav <gi...@git.apache.org>.
Github user aarondav commented on the pull request:
https://github.com/apache/spark/pull/3143#issuecomment-62209811
Right, I believe this is not busy-waiting, but a periodic task executing in a separate thread. Perhaps a ScheduledExecutorService would be clearer, but this seems like a reasonable use of volatile.
---
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-4282][YARN] Stopping flag in YarnClient...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/3143#issuecomment-62070678
[Test build #23019 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/23019/consoleFull) for PR 3143 at commit [`58fdcc9`](https://github.com/apache/spark/commit/58fdcc9024b97741e8fbaf7768dfc3ecfa9a3792).
* This patch **passes all tests**.
* This patch merges cleanly.
* This patch adds no public classes.
---
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-4282][YARN] Stopping flag in YarnClient...
Posted by tgravescs <gi...@git.apache.org>.
Github user tgravescs commented on the pull request:
https://github.com/apache/spark/pull/3143#issuecomment-62561021
I agree all we need is to make it volatile, no other synchronization is required. It sounds good to commonize the monitor logic inside of Client/ClientBase. Lets make sure to not have monitorApplication spin off an extra thread in cluster mode as it would just add extra overhead.
If we want to get this into 1.2 I would rather just have this bug fix the volatile flag, then file a separate jira to do the other cleanup.
---
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-4282][YARN] Stopping flag in YarnClient...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/spark/pull/3143
---
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-4282][YARN] Stopping flag in YarnClient...
Posted by aarondav <gi...@git.apache.org>.
Github user aarondav commented on the pull request:
https://github.com/apache/spark/pull/3143#issuecomment-62208964
Why does the thread interrupt itself after it's completed the while loop? There is nothing that would see this.
---
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-4282][YARN] Stopping flag in YarnClient...
Posted by aarondav <gi...@git.apache.org>.
Github user aarondav commented on the pull request:
https://github.com/apache/spark/pull/3143#issuecomment-62212819
That sounds good, but do note that that wouldn't be quite the same semantics, as the current pattern allows someone calling sc.stop() to terminate this thread cleanly, independent of what YARN says. Perhaps this is not a useful distinction, however.
---
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-4282][YARN] Stopping flag in YarnClient...
Posted by JoshRosen <gi...@git.apache.org>.
Github user JoshRosen commented on the pull request:
https://github.com/apache/spark/pull/3143#issuecomment-62210611
I'm not too familiar with the YARN code, but it looks like the `Client` has a `monitorApplication()` method that blocks until the application completes and returns its status. To avoid code duplication / improve understandability, couldn't we spin off a thread that calls that method (since that already implements the polling logic) and calls `sc.stop()` once that method returns? I think `sc.stop()` is thread-safe (or can be).
---
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-4282][YARN] Stopping flag in YarnClient...
Posted by sarutak <gi...@git.apache.org>.
Github user sarutak commented on the pull request:
https://github.com/apache/spark/pull/3143#issuecomment-62205789
Ah... embarrassing. Yes, we should use synchronization.
---
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-4282][YARN] Stopping flag in YarnClient...
Posted by JoshRosen <gi...@git.apache.org>.
Github user JoshRosen commented on the pull request:
https://github.com/apache/spark/pull/3143#issuecomment-62562295
@tgravescs That seems fine to me; you can go ahead and merge this, if you'd like, or I'll do it when I get back later this morning.
---
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-4282][YARN] Stopping flag in YarnClient...
Posted by tgravescs <gi...@git.apache.org>.
Github user tgravescs commented on the pull request:
https://github.com/apache/spark/pull/3143#issuecomment-62593801
+1, looks good. Filed SPARK-4346 to do the cleanup and commonization
---
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-4282][YARN] Stopping flag in YarnClient...
Posted by JoshRosen <gi...@git.apache.org>.
Github user JoshRosen commented on the pull request:
https://github.com/apache/spark/pull/3143#issuecomment-62205376
Why not use proper synchronization / waiting instead, or a semaphore?
---
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-4282][YARN] Stopping flag in YarnClient...
Posted by JoshRosen <gi...@git.apache.org>.
Github user JoshRosen commented on the pull request:
https://github.com/apache/spark/pull/3143#issuecomment-62209613
This looked like busy-waiting to me, which seems like an antipattern compared to waiting on a condition variable / semaphore, but I guess we need to poll YARN to find out whether the application has exited.
---
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-4282][YARN] Stopping flag in YarnClient...
Posted by aarondav <gi...@git.apache.org>.
Github user aarondav commented on the pull request:
https://github.com/apache/spark/pull/3143#issuecomment-62225761
SGTM
---
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-4282][YARN] Stopping flag in YarnClient...
Posted by aarondav <gi...@git.apache.org>.
Github user aarondav commented on the pull request:
https://github.com/apache/spark/pull/3143#issuecomment-62209142
Also, I'm not certain of the advantage of using proper synchronization over a volatile variable here, the latter seems sufficient to the task, unless you want to wait for the thread to terminate, which seems unnecessary since it's daemonic.
---
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org