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 memory revoke in TopNRowNumberOperator #22281

Merged

Conversation

hantangwangd
Copy link
Member

@hantangwangd hantangwangd commented Mar 21, 2024

Description

Fix #22127 .

TopNRowNumberOperator should not simply return a completed future for startMemoryRevoke() when it's flag finishing is true. That could lead in the problem described in #22127 when events occur in following order:

  1. Have successfully executed spilling at least once, so that the spiller is existing.
  2. Then handle some new input pages.
  3. Next, the finish() method is called by upstream operator, and the flag memoryRevokingRequested is set by MemoryRevokingScheduler.
  4. After that, when driver try to execute handleMemoryRevoke(), the error above would appear.

Test Plan

  • Existing test case TestDistributedSpilledQueriesWithTempStorage.testRowNumberLimit
  • Before this fix, this error almost certain to occur when running the test 2000 times in a loop. After this fix, it never occur again.

Contributor checklist

  • Please make sure your submission complies with our development, formatting, commit message, and attribution guidelines.
  • PR description addresses the issue accurately and concisely. If the change is non-trivial, a GitHub Issue is referenced.
  • Documented new properties (with its default value), SQL syntax, functions, or other functionality.
  • If release notes are required, they follow the release notes guidelines.
  • Adequate tests were added if applicable.
  • CI passed.

Release Notes

== RELEASE NOTES ==

General Changes
* Fix bug with spilling in TopNRowNumber

@hantangwangd hantangwangd requested a review from a team as a code owner March 21, 2024 17:22
@rschlussel
Copy link
Contributor

This should have a release note. it fixes a bug with spilling in TopNRowNumber

@hantangwangd
Copy link
Member Author

hantangwangd commented Mar 21, 2024

This should have a release note. it fixes a bug with spilling in TopNRowNumber

OK, done.

@hantangwangd hantangwangd force-pushed the fix_spill_in_topn_row_number branch from 36f1401 to 54e4af6 Compare March 21, 2024 23:28
@hantangwangd hantangwangd merged commit 6f1da7f into prestodb:master Mar 22, 2024
56 checks passed
@hantangwangd hantangwangd deleted the fix_spill_in_topn_row_number branch March 22, 2024 01:17
@wanglinsong wanglinsong mentioned this pull request May 1, 2024
48 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Flaky test TestDistributedSpilledQueriesWithTempStorage.testRowNumberLimit
4 participants