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

Fix unspill scheduling in join spill #9275

Merged
merged 1 commit into from Sep 24, 2021

Conversation

JunhyungSong
Copy link
Member

Comparable changes extracted from prestodb/presto#16339

Before the fix, unspill of next partition is
triggered when previous partition is released.
However, it just sends release request without
waiting for release to complete. As a result,
multiple partitions exist simultaneously and
result in memory limit error.

@cla-bot
Copy link

cla-bot bot commented Sep 17, 2021

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please submit the signed CLA to cla@trino.io. For more information, see https://github.com/trinodb/cla.

Copy link
Member

@sopel39 sopel39 left a comment

Choose a reason for hiding this comment

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

lgtm % small comments

public synchronized void setDisposeCompleted()
{
disposeCompleted.set(null);
setState(State.DISPOSE_COMPLETED);
Copy link
Member

Choose a reason for hiding this comment

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

can this be called before dispose by HashBuilderOperator?

Copy link
Member Author

Choose a reason for hiding this comment

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

why do you want to call this before dispose? Then the next page could be loaded before dispose is done.

@cla-bot
Copy link

cla-bot bot commented Sep 18, 2021

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please submit the signed CLA to cla@trino.io. For more information, see https://github.com/trinodb/cla.

@sopel39
Copy link
Member

sopel39 commented Sep 20, 2021

@JunhyungSong could you sign CLA please?

@JunhyungSong
Copy link
Member Author

@JunhyungSong could you sign CLA please?

I have done that. can you check that, please?

@sopel39
Copy link
Member

sopel39 commented Sep 23, 2021

@cla-bot check

@cla-bot cla-bot bot added the cla-signed label Sep 23, 2021
@cla-bot
Copy link

cla-bot bot commented Sep 23, 2021

The cla-bot has been summoned, and re-checked this pull request!

@sopel39
Copy link
Member

sopel39 commented Sep 23, 2021

Could you push empty commit?

Before the fix, unspill of next partition is
triggered when previous partition is released.
However, it just sends release request without
waiting for release to complete. As a result,
multiple partitions exist simultaneously and
result in memory limit error.
@JunhyungSong
Copy link
Member Author

Could you push empty commit?

Done.

@sopel39 sopel39 merged commit c754dfd into trinodb:master Sep 24, 2021
@sopel39 sopel39 mentioned this pull request Sep 24, 2021
8 tasks
@github-actions github-actions bot added this to the 363 milestone Sep 24, 2021
@JunhyungSong JunhyungSong deleted the join-unspill-schedulling branch September 25, 2021 02:41
@sopel39 sopel39 mentioned this pull request Oct 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

3 participants