Skip to content

Conversation

@kayousterhout
Copy link
Contributor

@squito this seemed like something we should just fix, independent of your change. Let me know what you think of this fix.

@SparkQA
Copy link

SparkQA commented Jul 8, 2015

Test build #36741 has finished for PR 7275 at commit e150278.

  • This patch fails Scala style tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@squito
Copy link
Contributor

squito commented Jul 8, 2015

Looks good (with the minor style fix) -- much clearer than it was before!

I was a little concerned about what would happen when you submit a job which re-uses a previously computed stage -- would the stage attempt ids inside the job start be consistent before and after this change? But I ran a small example and I realized it doesn't matter. If a shuffle output is reused, we dont' actually reuse the stage, we make a new one which gets skipped. (I thought there was some circumstance where two jobs could refer to the exact same stage, but I can't seem to recreate it now ... is it possible?)

Its kinda weird that job start even has the attempts for the stage, since they will always be 0, but I guess that is just an artifact of reusing the StageInfo object.

Copy link
Contributor

Choose a reason for hiding this comment

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

needs a return type

@SparkQA
Copy link

SparkQA commented Jul 10, 2015

Test build #37050 has finished for PR 7275 at commit 3e9ce7c.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@squito
Copy link
Contributor

squito commented Jul 11, 2015

lgtm

kayousterhout added a commit that referenced this pull request Jul 13, 2015
Author: Kay Ousterhout <[email protected]>

Closes #7275 from kayousterhout/SPARK-8880 and squashes the following commits:

3e9ce7c [Kay Ousterhout] Added missing return type
e150278 [Kay Ousterhout] [SPARK-8880] Fix confusing Stage.attemptId member variable
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants