Skip to content

[native] Add native memory pool reclaimer priority #25325

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.

Sign up for GitHub

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

Jump to bottom
Merged
singcha merged 1 commit into from
Jun 17, 2025
Merged

[native] Add native memory pool reclaimer priority #25325

singcha merged 1 commit into from
Jun 17, 2025
+34 −2

Conversation

@singcha
Copy link
Contributor

@singcha singcha commented Jun 13, 2025 •
edited
Loading

Description

Adds native_query_memory_reclaimer_priority property.
This is used to pass query memory reclaimer priority to Velox. Velox memory manager uses specified property to determine which query to kill when system runs on memory pressure.

Motivation and Context

When native worker runs into memory pressure, it starts killing queries to avoid cgroup OOM/process crash. When choosing the victim, we want to specify which queries are more likely to be chosen as victim. This change enables specifying memory reclaimer priority which is used for memory kills.
It could be further extended to choose which queries reclaimer spills first, which is outside the scope of this PR

Impact

Critical queries are less likely to be killed in a cluster

Test Plan

  • Unit test for property
  • Deployed on test cluster and replayed production traffic. Observed no critical queries getting killed.
== RELEASE NOTES ==

General Changes
* Add property ```native_query_memory_reclaimer_priority```  which controls which queries are killed first when a worker is running low on memory. Higher value means lower priority to be consistent with velox memory reclaimer's convention

@singcha singcha requested review from a team, elharo and steveburnett as code owners June 13, 2025 21:55
@singcha singcha requested a review from ZacBlanco June 13, 2025 21:56
@facebook-github-bot
Copy link
Collaborator

facebook-github-bot commented Jun 13, 2025

This pull request was exported from Phabricator. Differential Revision: D76368381

@singcha singcha requested review from amitkdutta and spershin June 13, 2025 21:57
@singcha
Copy link
Contributor Author

singcha commented Jun 13, 2025 •
edited by amitkdutta
Loading

depends on #25318
@amitkdutta @spershin

@amitkdutta amitkdutta changed the title add native memory pool reclaimer property [native] Add native memory pool reclaimer property Jun 13, 2025
@amitkdutta
Copy link
Contributor

amitkdutta commented Jun 13, 2025

CC: @aditi-pandit @czentgr This is waiting for velox advance PR #25318

@facebook-github-bot
Copy link
Collaborator

facebook-github-bot commented Jun 13, 2025

This pull request was exported from Phabricator. Differential Revision: D76368381

@singcha singcha force-pushed the export-D76368381 branch from 49e4914 to f0a00f8 Compare June 13, 2025 22:22
@aditi-pandit
Copy link
Contributor

aditi-pandit commented Jun 13, 2025

@amitkdutta : This looks like a new property... Is something existing broken for you folks until this makes in ?

@singcha singcha changed the title [native] Add native memory pool reclaimer property [native] Add native memory pool reclaimer priority Jun 13, 2025
steveburnett
steveburnett approved these changes Jun 13, 2025
View reviewed changes
Copy link
Contributor

@steveburnett steveburnett left a comment

Choose a reason for hiding this comment

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

LGMT! (docs)

Pull branch, local doc build, looks good. Thanks!

@amitkdutta
Copy link
Contributor

amitkdutta commented Jun 13, 2025

@amitkdutta : This looks like a new property... Is something existing broken for you folks until this makes in ?

Currently our production set up kills queries without priority, which is problematic. We want to roll out a feature where query can be killed by prirority. This is a major reliablity problem.

@aditi-pandit
Copy link
Contributor

aditi-pandit commented Jun 13, 2025

@amitkdutta : That makes sense. Its definitely an improvement. It doesn't sound like it's fixing a regression though.

Lets try to isolate which Velox change broke the build. That build is just bad... its possible nothing Prestissimo will run with it.

@aditi-pandit
Copy link
Contributor

aditi-pandit commented Jun 13, 2025 •
edited
Loading

@amitkdutta : Heard that Christian gave you a patch. Lets try it. I've removed the hold on the other PR.

@amitkdutta
Copy link
Contributor

amitkdutta commented Jun 14, 2025

@singcha Please rebase on master branch. We have merged #25326 which solves test failures and protocol issues both, and it advanced velox as well. Thanks

@facebook-github-bot
Copy link
Collaborator

facebook-github-bot commented Jun 16, 2025

This pull request was exported from Phabricator. Differential Revision: D76368381

@singcha singcha force-pushed the export-D76368381 branch from f0a00f8 to 7127750 Compare June 16, 2025 16:55
@singcha
Copy link
Contributor Author

singcha commented Jun 16, 2025

@amitkdutta @aditi-pandit - Updated. thanks for fixing the build.

@singcha @facebook-github-bot
Add native memory pool reclaimer property (prestodb#25325)
e215718 
Summary:
Pull Request resolved: prestodb#25325

add native_query_memory_reclaimer_priority property
Used for victim selection when killing queries to avoid worker OOM

Differential Revision: D76368381
@facebook-github-bot
Copy link
Collaborator

facebook-github-bot commented Jun 16, 2025

This pull request was exported from Phabricator. Differential Revision: D76368381

@singcha singcha force-pushed the export-D76368381 branch from 7127750 to e215718 Compare June 16, 2025 16:59
amitkdutta
amitkdutta approved these changes Jun 17, 2025
View reviewed changes
Copy link
Contributor

@amitkdutta amitkdutta left a comment

Choose a reason for hiding this comment

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

Thanks @singcha

@singcha singcha merged commit 4739e83 into prestodb:master Jun 17, 2025
170 of 172 checks passed
@singcha singcha deleted the export-D76368381 branch June 17, 2025 16:25
zoltan pushed a commit to zoltan/presto that referenced this pull request Jun 18, 2025
@singcha @zoltan
Add native memory pool reclaimer property (prestodb#25325)
9ccdcdd 
Summary:
Pull Request resolved: prestodb#25325

add native_query_memory_reclaimer_priority property
Used for victim selection when killing queries to avoid worker OOM

Differential Revision: D76368381
@unidevel unidevel mentioned this pull request Jun 19, 2025
Closed
6 tasks
anandamideShakyan pushed a commit to anandamideShakyan/presto that referenced this pull request Jun 19, 2025
@singcha @anandamideShakyan
Add native memory pool reclaimer property (prestodb#25325)
e8df97b 
Summary:
Pull Request resolved: prestodb#25325

add native_query_memory_reclaimer_priority property
Used for victim selection when killing queries to avoid worker OOM

Differential Revision: D76368381
This was referenced Jul 4, 2025
Merged
Closed
Closed
This was referenced Jul 24, 2025
Merged
Merged
@prestodb-ci prestodb-ci mentioned this pull request Jul 28, 2025
Merged
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@steveburnett steveburnett steveburnett approved these changes

@rschlussel rschlussel rschlussel approved these changes

@amitkdutta amitkdutta amitkdutta approved these changes

@elharo elharo Awaiting requested review from elharo elharo is a code owner

@ZacBlanco ZacBlanco Awaiting requested review from ZacBlanco ZacBlanco is a code owner automatically assigned from prestodb/committers

@spershin spershin Awaiting requested review from spershin

Assignees

No one assigned

Labels

fb-exported

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@singcha @facebook-github-bot @amitkdutta @aditi-pandit @steveburnett @rschlussel