Skip to content

Conversation

@zsxwing
Copy link
Member

@zsxwing zsxwing commented Aug 11, 2015

InternalAccumulator.create doesn't call registerAccumulatorForCleanup to register itself with ContextCleaner, so WeakReferences for these accumulators in Accumulators.originals won't be removed.

This PR added registerAccumulatorForCleanup for internal accumulators to avoid the memory leak.

@zsxwing
Copy link
Member Author

zsxwing commented Aug 11, 2015

/cc @andrewor14

@andrewor14
Copy link
Contributor

Good catch. LGTM will merge once tests pass. Thanks for fixing this.

@SparkQA
Copy link

SparkQA commented Aug 11, 2015

Test build #40460 timed out for PR 8108 at commit 1dfc567 after a configured wait of 175m.

@rxin
Copy link
Contributor

rxin commented Aug 11, 2015

I'm going to merge this since Jenkins actually ran all the tests.

@asfgit asfgit closed this in f16bc68 Aug 11, 2015
asfgit pushed a commit that referenced this pull request Aug 11, 2015
…Reference

`InternalAccumulator.create` doesn't call `registerAccumulatorForCleanup` to register itself with ContextCleaner, so `WeakReference`s for these accumulators in `Accumulators.originals` won't be removed.

This PR added `registerAccumulatorForCleanup` for internal accumulators to avoid the memory leak.

Author: zsxwing <[email protected]>

Closes #8108 from zsxwing/internal-accumulators-leak.

(cherry picked from commit f16bc68)
Signed-off-by: Reynold Xin <[email protected]>
@zsxwing zsxwing deleted the internal-accumulators-leak branch August 11, 2015 23:59
CodingCat pushed a commit to CodingCat/spark that referenced this pull request Aug 17, 2015
…Reference

`InternalAccumulator.create` doesn't call `registerAccumulatorForCleanup` to register itself with ContextCleaner, so `WeakReference`s for these accumulators in `Accumulators.originals` won't be removed.

This PR added `registerAccumulatorForCleanup` for internal accumulators to avoid the memory leak.

Author: zsxwing <[email protected]>

Closes apache#8108 from zsxwing/internal-accumulators-leak.
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.

4 participants