Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[SPARK-5460][MLlib] Wrapped Try around deleteAllCheckpoints - RandomForest. #4347

Closed
wants to merge 3 commits into from
Closed

[SPARK-5460][MLlib] Wrapped Try around deleteAllCheckpoints - RandomForest. #4347

wants to merge 3 commits into from

Conversation

x1-
Copy link
Contributor

@x1- x1- commented Feb 4, 2015

Because deleteAllCheckpoints has IOException potential.
fix issue.

@x1-
Wrapped Try around deleteAllCheckpoints - RandomForest.
1572576 
Because `deleteAllCheckpoints` has IOException potential.
@AmplabJenkins
Copy link

Can one of the admins verify this patch?

srowen
srowen reviewed Feb 4, 2015
View reviewed changes
mllib/src/main/scala/org/apache/spark/mllib/tree/RandomForest.scala
@@ -244,7 +245,10 @@ private class RandomForest (

// Delete any remaining checkpoints used for node Id cache.
if (nodeIdCache.nonEmpty) {
nodeIdCache.get.deleteAllCheckpoints()
Try(nodeIdCache.get.deleteAllCheckpoints()) match {
case Failure(e) => logWarning(s"delete all chackpoints faild. Error reason: ${e.getMessage}")
Copy link
Member

Choose a reason for hiding this comment

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

This has a typo ("faild"). Are you sure you want to continue in this case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@srowen
Thank you very much for indicating the typo.
Now, I modified typo and push again.

@mengxr
Copy link
Contributor

mengxr commented Feb 4, 2015

ok to test

@SparkQA
Copy link

SparkQA commented Feb 4, 2015

Test build #26767 has started for PR 4347 at commit 3a52745.

  • This patch merges cleanly.

@SparkQA
Copy link

SparkQA commented Feb 4, 2015

Test build #26767 has finished for PR 4347 at commit 3a52745.

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

@AmplabJenkins
Copy link

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

mengxr
mengxr reviewed Feb 4, 2015
View reviewed changes
mllib/src/main/scala/org/apache/spark/mllib/tree/RandomForest.scala
@@ -244,7 +245,10 @@ private class RandomForest (

// Delete any remaining checkpoints used for node Id cache.
if (nodeIdCache.nonEmpty) {
nodeIdCache.get.deleteAllCheckpoints()
Try(nodeIdCache.get.deleteAllCheckpoints()) match {
case Failure(e) => logWarning(s"delete all chackpoints failed. Error reason: ${e.getMessage}")
Copy link
Contributor

Choose a reason for hiding this comment

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

line too wide. Move logWarning to the next line.

Btw, I don't see how Try is useful here. Maybe we should just use

try {
  ...
} catch {
  case ...
}

Java users would feel familiar with the code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mengxr
Thank you ❗ and I see that.
So, change and push the code.

@SparkQA
Copy link

SparkQA commented Feb 5, 2015

Test build #26832 has started for PR 4347 at commit cdd3fa2.

  • This patch merges cleanly.

@SparkQA
Copy link

SparkQA commented Feb 5, 2015

Test build #26832 has finished for PR 4347 at commit cdd3fa2.

  • This patch fails to build.
  • This patch merges cleanly.
  • This patch adds no public classes.

@AmplabJenkins
Copy link

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

@x1-
Copy link
Contributor Author

x1- commented Feb 5, 2015

test this please

@SparkQA
Copy link

SparkQA commented Feb 5, 2015

Test build #26835 has started for PR 4347 at commit 7a3d8de.

  • This patch merges cleanly.

@SparkQA
Copy link

SparkQA commented Feb 5, 2015

Test build #26835 has finished for PR 4347 at commit 7a3d8de.

  • 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/26835/
Test PASSed.

asfgit pushed a commit that referenced this pull request Feb 5, 2015
@x1- @mengxr
[SPARK-5460][MLlib] Wrapped Try around deleteAllCheckpoints - Ran…
44768f5 
…domForest.

Because `deleteAllCheckpoints` has IOException potential.
fix issue.

Author: x1- <viva008@gmail.com>

Closes #4347 from x1-/SPARK-5460 and squashes the following commits:

7a3d8de [x1-] change `Try()` to `try catch { case ... }` ar RandomForest.
3a52745 [x1-] modified typo. 'faild' -> 'failed' and remove disused '-'.
1572576 [x1-] Wrapped `Try` around `deleteAllCheckpoints` - RandomForest.

(cherry picked from commit 62371ad)
Signed-off-by: Xiangrui Meng <meng@databricks.com>
@asfgit asfgit closed this in 62371ad Feb 5, 2015
@mengxr
Copy link
Contributor

mengxr commented Feb 5, 2015

LGTM. Merged into master and branch-1.3. Thanks!

@x1-
Copy link
Contributor Author

x1- commented Feb 6, 2015

@mengxr
Thank you very much 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
5 participants
@x1- @AmplabJenkins @mengxr @SparkQA @srowen