You are viewing a plain text version of this content. The canonical link for it is here.

Posted to reviews@spark.apache.org by mallman <gi...@git.apache.org> on 2016/01/11 20:15:18 UTC

[GitHub] spark pull request: [SPARK-6950][CORE] Stop the event logger befor...

GitHub user mallman opened a pull request:

    https://github.com/apache/spark/pull/10700

    [SPARK-6950][CORE] Stop the event logger before the DAG scheduler

    [SPARK-6950][CORE] Stop the event logger before the DAG scheduler to avoid a race condition where the standalone master attempts to build the app's history UI before the event log is stopped

You can merge this pull request into a Git repository by running:

    $ git pull https://github.com/VideoAmp/spark-public stop_event_logger_first

Alternatively you can review and apply these changes as the patch at:

    https://github.com/apache/spark/pull/10700.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 #10700
    
----
commit 7085daa95f96bcf65447e4bb5f39d9014a663614
Author: Michael Allman <mi...@videoamp.com>
Date:   2016-01-11T19:13:01Z

    [SPARK-6950][CORE] Stop the event logger before the DAG scheduler to
    avoid a race condition where the standalone master attempts to build the
    app's history UI before the event log is stopped

----


---
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-12755][CORE] Stop the event logger befo...

Posted by SparkQA <gi...@git.apache.org>.

Github user SparkQA commented on the pull request:

    https://github.com/apache/spark/pull/10700#issuecomment-172530317
  
    **[Test build #2397 has started](https://amplab.cs.berkeley.edu/jenkins/job/NewSparkPullRequestBuilder/2397/consoleFull)** for PR 10700 at commit [`6c32f77`](https://github.com/apache/spark/commit/6c32f77890d8ac5f2117020a65167e27124aeae5).


---
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-12755][CORE] Stop the event logger befo...

Posted by mallman <gi...@git.apache.org>.

Github user mallman commented on the pull request:

https://github.com/apache/spark/pull/10700#issuecomment-173490509

Here are my current thoughts. Josh says this functionality is going to be removed in Spark 2.0. The bug this PR is designed to address manifests itself in Spark 1.5 in three ways (I'm aware of):

1. Misleading log messages from the Master (reported above).
2. Incomplete (aka "in progress") application event logs, which can be further divided into two scenarios:
2.a. Incomplete uncompressed event log files. The log processor can recover these files.
2.b. Incomplete compressed event log files. The compression output is truncated and unreadable by normal means. The history server reports a corrupted event log. I cannot definitively tie that symptom to this bug, but it agrees with my experience.

The most problematic of these is unrecoverable event logs. I've been frustrated by this before and turned off event log compression as a workaround. Since deploying a build with this patch to one of our dev clusters I haven't seen this problem again.

I don't see a simple way to write a test to support this PR.

Overall, I feel we should close this PR but keep a reference to it from Jira with a comment that Spark 1.5 and 1.6 users can try this patch—at their own risk—to address the described symptoms if they wish to. It's going into our own Spark 1.x builds.

I'll close this PR and the associated Jira issue within the next few days unless someone objects or wishes to continue discussion.

Thanks.

---
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-12755][CORE] Stop the event logger befo...

Posted by srowen <gi...@git.apache.org>.

Github user srowen commented on the pull request:

    https://github.com/apache/spark/pull/10700#issuecomment-173505157
  
    Are there downsides to merging this to master, even if the related functionality is about to be removed? it passes tests, and seems to improve an ordering of shutdown, and can be backported to fix an actual minor issue in previous releases. Tests would be cool but you're correct that this one could be really hard to trigger. I see no reason to close 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-12755][CORE] Stop the event logger befo...

Posted by mallman <gi...@git.apache.org>.

Github user mallman commented on the pull request:

    https://github.com/apache/spark/pull/10700#issuecomment-172564021
  
    Sorry guys. I bungled the ordering of the `stop()` calls. That's what I get for doing a manual patch from a manual diff from another branch-1.5... :disappointed: This patch passes all tests on branch-1.3. Can you please kick off a new test build in jenkins?


---
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-12755][CORE] Stop the event logger befo...

Posted by asfgit <gi...@git.apache.org>.

Github user asfgit closed the pull request at:

    https://github.com/apache/spark/pull/10700


---
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-12755][CORE] Stop the event logger befo...

Posted by SparkQA <gi...@git.apache.org>.

Github user SparkQA commented on the pull request:

    https://github.com/apache/spark/pull/10700#issuecomment-172574018
  
    **[Test build #2398 has started](https://amplab.cs.berkeley.edu/jenkins/job/NewSparkPullRequestBuilder/2398/consoleFull)** for PR 10700 at commit [`57cade1`](https://github.com/apache/spark/commit/57cade176fa176d6642a78e2f16430ffdca19a6e).


---
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-12755][CORE] Stop the event logger befo...

Posted by mallman <gi...@git.apache.org>.

Github user mallman commented on the pull request:

https://github.com/apache/spark/pull/10700#issuecomment-172443532

I should also state that my original motivation in submitting this patch was to address the confusing log messages

Application ... is still in progress, it may be terminated abnormally.

which I saw in the Spark master log for app's that actually terminated normally.

Also, it's just come to mind that this bug may explain another behavior I've seen—sometimes an app's event log is corrupted if it was configured to be compressed. If the log is uncompressed then the ability for the history reader to decode an "in progress" event log allows it to be processed as expected. However, if the event log is being written through a compressed output stream and is not properly flushed before it is processed, then the processing may fail because the file compression was incomplete. (As this just occurred to me I haven't tested this hypothesis, but it does sound like a plausible explanation.) If this is the case, then this patch should correct the problem with corrupt compressed event logs.

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] spark pull request: [SPARK-12755][CORE] Stop the event logger befo...

Posted by SparkQA <gi...@git.apache.org>.

Github user SparkQA commented on the pull request:

    https://github.com/apache/spark/pull/10700#issuecomment-172608176
  
    **[Test build #2398 has finished](https://amplab.cs.berkeley.edu/jenkins/job/NewSparkPullRequestBuilder/2398/consoleFull)** for PR 10700 at commit [`57cade1`](https://github.com/apache/spark/commit/57cade176fa176d6642a78e2f16430ffdca19a6e).
     * 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-12755][CORE] Stop the event logger befo...

Posted by srowen <gi...@git.apache.org>.

Github user srowen commented on the pull request:

    https://github.com/apache/spark/pull/10700#issuecomment-172483987
  
    Yes, we'd prefer to make changes in `master` and then back port rather than apply only to a branch. I think the change is at worst a no-op for `master`? if so I think this change is fine. It sounds like it needs to be back-ported to 1.5.


---
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-12755][CORE] Stop the event logger befo...

Posted by mallman <gi...@git.apache.org>.

Github user mallman commented on the pull request:

    https://github.com/apache/spark/pull/10700#issuecomment-174599806
  
    Thanks, @srowen.


---
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-6950][CORE] Stop the event logger befor...

Posted by AmplabJenkins <gi...@git.apache.org>.

Github user AmplabJenkins commented on the pull request:

    https://github.com/apache/spark/pull/10700#issuecomment-170659269
  
    Can one of the admins verify this patch?


---
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-12755][CORE] Stop the event logger befo...

Posted by mallman <gi...@git.apache.org>.

Github user mallman commented on the pull request:

https://github.com/apache/spark/pull/10700#issuecomment-172440507

Hi Josh,

Good questions. I may have submitted this PR incorrectly. Perhaps you can guide me in the right direction.

I submitted this PR for merging into master because my understanding is that's how all PR's for the Spark project should be created. And patches against master may be backported to earlier releases. However, I originally created and tested this patch on branch-1.5 because that's what we're currently running. So while this patch may be irrelevant to master (or Spark 2.0), it's relevant to the Spark 1.5 branch and presumably 1.6 as well. Under these circumstances, should I have submitted a PR against master as I have done? The code contribution guidelines state that only in a special case would a PR be opened against another branch. Does a patch with no or lesser relevance to the master branch compared to an earlier release branch qualify as a "special case"? And if so, which branch should I have submitted the PR against?

Thanks.

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] spark pull request: [SPARK-12755][CORE] Stop the event logger befo...

Posted by SparkQA <gi...@git.apache.org>.

Github user SparkQA commented on the pull request:

    https://github.com/apache/spark/pull/10700#issuecomment-172557308
  
    **[Test build #2397 has finished](https://amplab.cs.berkeley.edu/jenkins/job/NewSparkPullRequestBuilder/2397/consoleFull)** for PR 10700 at commit [`6c32f77`](https://github.com/apache/spark/commit/6c32f77890d8ac5f2117020a65167e27124aeae5).
     * This patch **fails Spark unit 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-12755][CORE] Stop the event logger befo...

Posted by SparkQA <gi...@git.apache.org>.

Github user SparkQA commented on the pull request:

    https://github.com/apache/spark/pull/10700#issuecomment-172484488
  
    **[Test build #2395 has started](https://amplab.cs.berkeley.edu/jenkins/job/NewSparkPullRequestBuilder/2395/consoleFull)** for PR 10700 at commit [`6c32f77`](https://github.com/apache/spark/commit/6c32f77890d8ac5f2117020a65167e27124aeae5).


---
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-12755][CORE] Stop the event logger befo...

Posted by JoshRosen <gi...@git.apache.org>.

Github user JoshRosen commented on the pull request:

    https://github.com/apache/spark/pull/10700#issuecomment-171442415
  
    Will this change still be relevant if we remove the Spark Master's embedded HistoryServer? In other words, does this race condition affect the standalone HistoryServer or only the Master history server? If it only affects the master then this isn't worth changing in master since we're going to remove the master's embedded history server for Spark 2.0. It may still be worthwhile for Spark 1.6, though, although there's a risk/benefit trade-off 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-12755][CORE] Stop the event logger befo...

Posted by srowen <gi...@git.apache.org>.

Github user srowen commented on the pull request:

    https://github.com/apache/spark/pull/10700#issuecomment-174454362
  
    Merged to master/1.6/1.5


---
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-6950][CORE] Stop the event logger befor...

Posted by mallman <gi...@git.apache.org>.

Github user mallman commented on the pull request:

    https://github.com/apache/spark/pull/10700#issuecomment-170659870
  
    Changed Jira ref from SPARK-6950 to SPARK-12755. SPARK-6950 is an older, defunct ticket. Oops.


---
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-12755][CORE] Stop the event logger befo...

Posted by JoshRosen <gi...@git.apache.org>.

Github user JoshRosen commented on a diff in the pull request:

    https://github.com/apache/spark/pull/10700#discussion_r49654293
  
    --- Diff: core/src/main/scala/org/apache/spark/SparkContext.scala ---
    @@ -1681,6 +1681,9 @@ class SparkContext(config: SparkConf) extends Logging with ExecutorAllocationCli
         Utils.tryLogNonFatalError {
           _executorAllocationManager.foreach(_.stop())
         }
    +    Utils.tryLogNonFatalError {
    --- End diff --
    
    Maybe add a comment here to explain why this needs to be stopped before the DAGScheduler in order to prevent this change from being accidentally lost in the future?


---
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-12755][CORE] Stop the event logger befo...

Posted by SparkQA <gi...@git.apache.org>.

Github user SparkQA commented on the pull request:

    https://github.com/apache/spark/pull/10700#issuecomment-172503953
  
    **[Test build #2395 has finished](https://amplab.cs.berkeley.edu/jenkins/job/NewSparkPullRequestBuilder/2395/consoleFull)** for PR 10700 at commit [`6c32f77`](https://github.com/apache/spark/commit/6c32f77890d8ac5f2117020a65167e27124aeae5).
     * This patch **fails Spark unit 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