Skip to content

Conversation

@piaozhexiu
Copy link

No description provided.

@AmplabJenkins
Copy link

Can one of the admins verify this patch?

Copy link
Contributor

Choose a reason for hiding this comment

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

So, the problem is that this was not catching Error, but just Exception, right?

I think this generates a compiler warning, so probably should be case _: Throwable =>. And you can avoid the duplicate getCause calls by giving this variable a name instead of using the placeholder.

Copy link
Member

Choose a reason for hiding this comment

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

Nit: reusing the name e is ambiguous but also unnecessary; this can be _ too? (Yes the second clause will generate a warning without : Throwable

@piaozhexiu
Copy link
Author

Thank you Marcelo and Sean. Incorporated your comments in the new commit.

Copy link
Member

Choose a reason for hiding this comment

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

e still shadows the e: InvocationTargetException above. I know it was already that way in the code and it works as intended this way. However while we're here I wonder if it's worth calling this variable cause? then it's obvious that cause is being rethrown.

Copy link
Author

Choose a reason for hiding this comment

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

Sure. That is even better. Fixed.

@srowen
Copy link
Member

srowen commented Feb 26, 2015

ok to test

@SparkQA
Copy link

SparkQA commented Feb 26, 2015

Test build #27967 has started for PR 4773 at commit 2a919d5.

  • This patch merges cleanly.

@SparkQA
Copy link

SparkQA commented Feb 26, 2015

Test build #27967 has finished for PR 4773 at commit 2a919d5.

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

@AmplabJenkins
Copy link

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/27967/
Test PASSed.

@srowen
Copy link
Member

srowen commented Feb 26, 2015

LGTM. Let me leave it open for a day at least for other thoughts.

@andrewor14
Copy link
Contributor

Ah, good catch. For a second I thought we were catching throwable here, but it seems that the throwable is part of the cause, not what we're catching. LGTM I'm merging this into master 1.3 and 1.2.

asfgit pushed a commit that referenced this pull request Feb 26, 2015
…RN AM

Author: Cheolsoo Park <[email protected]>

Closes #4773 from piaozhexiu/SPARK-6018 and squashes the following commits:

2a919d5 [Cheolsoo Park] Rename e with cause to avoid duplicate names
1e71d2d [Cheolsoo Park] Replace placeholder with throwable
eb5750d [Cheolsoo Park] NoSuchMethodError in Spark app is swallowed by YARN AM

(cherry picked from commit 5f3238b)
Signed-off-by: Andrew Or <[email protected]>
asfgit pushed a commit that referenced this pull request Feb 26, 2015
…RN AM

Author: Cheolsoo Park <[email protected]>

Closes #4773 from piaozhexiu/SPARK-6018 and squashes the following commits:

2a919d5 [Cheolsoo Park] Rename e with cause to avoid duplicate names
1e71d2d [Cheolsoo Park] Replace placeholder with throwable
eb5750d [Cheolsoo Park] NoSuchMethodError in Spark app is swallowed by YARN AM

(cherry picked from commit 5f3238b)
Signed-off-by: Andrew Or <[email protected]>
@asfgit asfgit closed this in 5f3238b Feb 26, 2015
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.

6 participants