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.

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

Wrap aggregated memory context in InternalAggregatedMemoryContext #764

Merged
merged 2 commits into from May 14, 2019

Conversation

2 participants
@sopel39
Copy link
Member

commented May 13, 2019

Otherwise peak memory accounting could be leaking if child aggregated memory context is used

@sopel39 sopel39 requested a review from dain May 13, 2019

@cla-bot cla-bot bot added the cla-signed label May 13, 2019

@dain

dain approved these changes May 13, 2019

@@ -675,7 +675,7 @@ public void close()
@Override
public AggregatedMemoryContext newAggregatedMemoryContext()
{
return delegate.newAggregatedMemoryContext();
return new InternalAggregatedMemoryContext(delegate.newAggregatedMemoryContext(), memoryFuture, allocationListener, true);

This comment has been minimized.

Copy link
@dain

dain May 13, 2019

Member

Can you add a better commit message explaining why?

Wrap aggregated memory context in InternalAggregatedMemoryContext
Peak memory accounting might be leaking if unwrapped
child aggregated memory context is used.

@sopel39 sopel39 force-pushed the starburstdata:ks/wrap_aggregated branch from afb37c4 to c9ff6c6 May 14, 2019

@sopel39 sopel39 merged commit d0dda28 into prestosql:master May 14, 2019

1 of 2 checks passed

Travis CI - Pull Request Build Created
Details
verification/cla-signed
Details

@sopel39 sopel39 deleted the starburstdata:ks/wrap_aggregated branch May 14, 2019

@sopel39 sopel39 referenced this pull request May 14, 2019

Closed

Release notes for 311 #716

5 of 5 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.