[SPARK-7100][MLLib] Fix persisted RDD leak in GradientBoostTrees

[jimfcarroll]

This fixes a leak of a persisted RDD where GradientBoostTrees can call persist but never unpersists.

Jira: https://issues.apache.org/jira/browse/SPARK-7100

Discussion: http://apache-spark-developers-list.1001551.n3.nabble.com/GradientBoostTrees-leaks-a-persisted-RDD-td11750.html

[srowen]

@jkbradley it's an interesting question -- it makes sense to ensure the RDD is unpersisted with try-finally but I don't know if any other code does it. I think the assumption has been that the app will soon die anyway if something unexpected is going wrong like an exception, so RDD cleanup isn't that important. Should we stick to that imperfect reasoning here and keep the code simpler?

[jimfcarroll]

I override and intercept most RDD calls in my own code to optimize them (which is how I found this in the first place) and we use try-with-resource to persist and unpersist. It's too bad you cant do that simply in scala.

If you want the simpler solution let me know. I can change it.

[jkbradley]

I too think it makes some sense but have not seen it done elsewhere. I think I asked about it when I first started working on Spark and was told something like @srowen said. Even if it's "leaked," other RDDs can still push it out of memory or disk if needed, right? Have you encountered cases where that does not happen?

[srowen]

ok to test

[SparkQA]

Test build #704 has finished for PR 5669 at commit e5be57c.

• This patch passes all tests.
• This patch merges cleanly.
• This patch adds no public classes.
• This patch does not change any dependencies.

[jkbradley]

After discussing with @mengxr I think we should not bother with the try-finally wrapper. As mentioned above, the method should generally not fail, so the data will be unpersisted as needed. When an exception is thrown, then the data will be unpersisted whenever another RDD pushes "input" out of memory/disk, without undue harm to other jobs.

@jimfcarroll Could you please update the PR to remove the try-finally wrapper, but keep the unpersist at the end?

Thanks for going through this discussion!

[jimfcarroll]

Your project. I'll downgrade it if you want. :-)

[jkbradley]

OK thank you. Sometimes, a slight improvement can be outweighed by the extra complexity.

[jimfcarroll]

Okay. I force pushed a different commit. I removed the try-finally.

Hope this works. Thanks.

[SparkQA]

Test build #723 has started for PR 5669 at commit 45f4b03.

[srowen]

This test actually passed, but Jenkins failed to post. LGTM.

data: {"body": " Test build #723 has finished for PR 5669 at commit 45f4b03.\n * This patch passes all tests.\n * This patch merges cleanly.\n * This patch adds no public classes.\nYour branch is ahead of 'origin/master' by 295 commits.

• This patch does not change any dependencies."}