Skip to content

Remove support for reserved memory pool#11064

Closed
losipiuk wants to merge 1 commit intotrinodb:masterfrom
losipiuk:lo/drop-pools
Closed

Remove support for reserved memory pool#11064
losipiuk wants to merge 1 commit intotrinodb:masterfrom
losipiuk:lo/drop-pools

Conversation

@losipiuk
Copy link
Copy Markdown
Member

@losipiuk losipiuk commented Feb 16, 2022

Description

Remove support for reserved memory pool.
Mechanism did not prove to be useful and was disabled by default.
Removing to simplify codebase.

Note: as now we have just one memory pool (general) a follow-up PR will remove the memory pools concept altogether.

Is this change a fix, improvement, new feature, refactoring, or other?

Improvment

Is this a change to the core query engine, a connector, client library, or the SPI interfaces? (be specific)

Core query engine

Related issues, pull requests, and links

Fixes #6677

Documentation

(x) No documentation is needed.
( ) Sufficient documentation is included in this PR.
( ) Documentation PR is available with #prnumber.
( ) Documentation issue #issuenumber is filed, and can be handled later.

Release notes

( ) No release notes entries required.
(x) Release notes entries required with the following suggested text:

# General
* Remove support for reserved memory pool. Configuration property `experimental.reserved-pool-disabled` can no longer be used. ({issue}`6677`)

@losipiuk losipiuk marked this pull request as ready for review February 17, 2022 10:15
@losipiuk losipiuk requested review from dain and martint February 17, 2022 10:16
@losipiuk
Copy link
Copy Markdown
Member Author

@findepi @arhimondr this one should be good to review.

@findepi
Copy link
Copy Markdown
Member

findepi commented Feb 17, 2022

Related issues, pull requests, and links

#6677

"Fixes #6677" ?


for (MemoryPoolAssignment assignment : assignments.getAssignments()) {
if (assignment.getPoolId().equals(GENERAL_POOL)) {
queryContexts.getUnchecked(assignment.getQueryId()).setMemoryPool(localMemoryManager.getGeneralPool());
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

is this a good reason to combine this with #11071 together as single change?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yeah - could be. I wanted to keep them separate for sake of easier review (the followup is mostly a code cleanup with no user-visible changes). But I may squash them together if you prefer.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

i reviewed #11071 instead of this one

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I will close this one then

@losipiuk
Copy link
Copy Markdown
Member Author

Lets just keep #11071

@losipiuk losipiuk closed this Feb 17, 2022
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.

Remove reserved pool

2 participants