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.

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

Remove distinction between system and user memory #10574

Merged
merged 1 commit into from
Jan 24, 2022
Merged

Remove distinction between system and user memory #10574

merged 1 commit into from
Jan 24, 2022

Conversation

findepi
Copy link
Member

@findepi findepi commented Jan 12, 2022

No description provided.

@findepi
Copy link
Member Author

findepi commented Jan 12, 2022

cc @erichwang

@sopel39 sopel39 requested a review from martint January 12, 2022 14:03
@losipiuk
Copy link
Member

❤️

@findepi findepi force-pushed the findepi/split-hair-on-memory branch from ca02083 to cda4433 Compare January 13, 2022 13:21
@cla-bot cla-bot bot added the cla-signed label Jan 13, 2022
@findepi findepi force-pushed the findepi/split-hair-on-memory branch from cda4433 to e67a592 Compare January 13, 2022 14:06
@findepi findepi force-pushed the findepi/split-hair-on-memory branch 3 times, most recently from b6f514c to 40046c0 Compare January 14, 2022 11:53
@losipiuk
Copy link
Member

trino/core/trino-main/src/main/java/io/trino/memory/MemoryManagerConfig.java

Lines 38 to 41 in cea5842

// enforced against user memory allocations
private DataSize maxQueryMemory = DataSize.of(20, GIGABYTE);
// enforced against user + system memory allocations (default is maxQueryMemory * 2)
private DataSize maxQueryTotalMemory;

remainder?

@losipiuk
Copy link
Member

https://github.com/trinodb/trino/blob/40046c0661056c15571e03be0bfaac163d0ade79/plugin/trino-hive/src/main/java/io/trino/plugin/hive/orc/OrcDeletedRows.java#L68

raname

@losipiuk
Copy link
Member

QueryStats still has user/total distincition. On purpose?

@findepi
Copy link
Member Author

findepi commented Jan 14, 2022

trino/core/trino-main/src/main/java/io/trino/memory/MemoryManagerConfig.java

Lines 38 to 41 in cea5842

// enforced against user memory allocations
private DataSize maxQueryMemory = DataSize.of(20, GIGABYTE);
// enforced against user + system memory allocations (default is maxQueryMemory * 2)
private DataSize maxQueryTotalMemory;

remainder?

No. The from cluster memory manager perspective, total memory = user + revocable.
I've updated the docs to make it clear.

QueryStats still has user/total distincition. On purpose?

same

losipiuk
losipiuk reviewed Jan 14, 2022
View reviewed changes
core/trino-main/src/main/java/io/trino/execution/SqlTaskManager.java
@@ -156,15 +153,13 @@ public SqlTaskManager(

this.localMemoryManager = requireNonNull(localMemoryManager, "localMemoryManager is null");
DataSize maxQueryMemoryPerNode = nodeMemoryConfig.getMaxQueryMemoryPerNode();
DataSize maxQueryTotalMemoryPerNode = nodeMemoryConfig.getMaxQueryTotalMemoryPerNode();
queryMaxMemoryPerTask = nodeMemoryConfig.getMaxQueryTotalMemoryPerTask();
queryMaxMemoryPerTask = nodeMemoryConfig.getMaxQueryMemoryPerTask();
Copy link
Member

Choose a reason for hiding this comment

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

preexistent - can you still rename this to maxQueryMaxMemoryPerTask

Copy link
Member Author

Choose a reason for hiding this comment

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

you mean to swap "query" and "max"? let's follow up

@findepi findepi force-pushed the findepi/split-hair-on-memory branch from af8fe86 to cf6370a Compare January 14, 2022 13:16
@findepi
Copy link
Member Author

findepi commented Jan 14, 2022

(squashed)

@findepi
Copy link
Member Author

findepi commented Jan 15, 2022

CI #10631, was green before squash.
rerunning.

@findepi findepi force-pushed the findepi/split-hair-on-memory branch from 1ab032c to aae7ab8 Compare January 21, 2022 17:34
@findepi
Copy link
Member Author

findepi commented Jan 21, 2022

(rebased)

@findepi findepi force-pushed the findepi/split-hair-on-memory branch 2 times, most recently from e0a4a82 to b3b9b8d Compare January 21, 2022 20:19
@findepi findepi force-pushed the findepi/split-hair-on-memory branch from b3b9b8d to 796544b Compare January 21, 2022 20:23
@github-actions github-actions bot added the docs label Jan 21, 2022
@findepi findepi merged commit 9ab09e9 into trinodb:master Jan 24, 2022
@findepi findepi deleted the findepi/split-hair-on-memory branch January 24, 2022 11:27
@findepi findepi mentioned this pull request Jan 24, 2022
Closed
@github-actions github-actions bot added this to the 369 milestone Jan 24, 2022
@mosabua mosabua mentioned this pull request Jan 24, 2022
Merged
@Praveen2112 Praveen2112 mentioned this pull request Feb 9, 2022
Closed
@miniway miniway mentioned this pull request Feb 9, 2022
Merged
@walter-weinmann walter-weinmann mentioned this pull request Feb 27, 2022
Closed
@maltesander maltesander mentioned this pull request Apr 12, 2022
Closed
7 tasks
@ElapsedSoul
Copy link

Old configurations are:

max-total-memory-per-node=40GB
max-memory-per-node=32GB

which value should max-memory-per-node now be?
32 or 40?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@losipiuk losipiuk losipiuk approved these changes

@arhimondr arhimondr arhimondr approved these changes

@dain dain Awaiting requested review from dain

@sopel39 sopel39 Awaiting requested review from sopel39

@martint martint Awaiting requested review from martint

Assignees
No one assigned
Labels
cla-signed docs
Milestone
369
Development

Successfully merging this pull request may close these issues.
4 participants
@findepi @losipiuk @ElapsedSoul @arhimondr