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 mechanism for supporting multiple memory pools #11071

Merged
merged 2 commits into from
Feb 18, 2022
Merged

Remove mechanism for supporting multiple memory pools #11071

merged 2 commits into from
Feb 18, 2022

Conversation

losipiuk
Copy link
Member

@losipiuk losipiuk commented Feb 16, 2022 •
edited
Loading

Description

Remove support for reserved memory pool.
Mechanism did not prove to be useful and was disabled by default.
Removing to simplify codebase.
After getting rid of the reserved pool we have just one pull and we can remove the mechanism for supporting multiple memory pool 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)

query engine, SPI

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`)

@cla-bot cla-bot bot added the cla-signed label Feb 16, 2022
@losipiuk losipiuk force-pushed the lo/drop-pools-concept branch from 65d25c4 to 3af6d4e Compare February 17, 2022 10:22
@losipiuk losipiuk marked this pull request as ready for review February 17, 2022 10:22
@losipiuk losipiuk requested review from martint and dain February 17, 2022 10:23
@losipiuk
Copy link
Member Author

@findepi PTAL at this one too.

@findepi findepi mentioned this pull request Feb 17, 2022
Closed
findepi
findepi approved these changes Feb 17, 2022
View reviewed changes
core/trino-main/src/main/java/io/trino/execution/MemoryRevokingScheduler.java Outdated Show resolved Hide resolved
core/trino-main/src/main/java/io/trino/memory/ClusterMemoryManager.java Outdated Show resolved Hide resolved
core/trino-main/src/main/java/io/trino/memory/ClusterMemoryManager.java Outdated Show resolved Hide resolved
core/trino-main/src/main/java/io/trino/memory/ClusterMemoryManager.java Show resolved Hide resolved
core/trino-main/src/main/java/io/trino/memory/ClusterMemoryManager.java Outdated Show resolved Hide resolved
core/trino-main/src/main/java/io/trino/memory/LocalMemoryManagerExporter.java Outdated
private final MBeanExporter exporter;
@GuardedBy("this")
private final List<MemoryPool> pools = new ArrayList<>();
private MemoryPool pool;
Copy link
Member

Choose a reason for hiding this comment

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

final.
still @GuardedBy("this")?

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree. Also it feels we did not need that synchronization before neither (give destroy is called once)

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually we do not need pool field at all - just a boolean flag if we exported JMX bean.

core/trino-main/src/main/java/io/trino/memory/NodeMemoryConfig.java Show resolved Hide resolved
core/trino-main/src/test/java/io/trino/memory/LowMemoryKillerTestingUtils.java Outdated Show resolved Hide resolved
...src/test/java/io/trino/plugin/resourcegroups/db/TestDbResourceGroupConfigurationManager.java Outdated Show resolved Hide resolved
testing/trino-tests/src/test/java/io/trino/memory/TestMemoryManager.java Outdated Show resolved Hide resolved
@losipiuk losipiuk force-pushed the lo/drop-pools-concept branch from 3af6d4e to 1a662a3 Compare February 17, 2022 13:19
@losipiuk
Copy link
Member Author

@findepi AC + some more code bashed. No rebase so you can just take a look at diff

@losipiuk losipiuk force-pushed the lo/drop-pools-concept branch from 369b6f6 to 5c61526 Compare February 17, 2022 16:42
losipiuk
losipiuk commented Feb 17, 2022
View reviewed changes
core/trino-main/src/main/java/io/trino/memory/MemoryResource.java
@ResourceSecurity(MANAGEMENT_READ)
@GET
@Path("{poolId}")
public Response getMemoryInfo(@PathParam("poolId") String poolId)
Copy link
Member Author

@losipiuk losipiuk Feb 17, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

@dain @findepi @arhimondr
I am a bit concerned about the removal of this endpoint. What was the target audience? Can it backfire?
getMemoryInfo above changed from POST to GET has the relevant information but it is tagged with INTERNAL_ONLY security so cannot be used as a replacement.
And I am also not sure if I can safely change INTERNAL_ONLY to MANAGEMENT_READ above.

Would appreciate some help here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe I can keep it with paramter and require that caller always has to pass "general" ?

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure who uses that. Maybe @electrum does? If you do what to keep it, change the annotation to @Path("general").

Copy link
Member

Choose a reason for hiding this comment

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

I think this was for internal monitoring at Facebook. I'm fine with removing it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks guys:)

@linzebing linzebing self-requested a review February 17, 2022 17:40
arhimondr
arhimondr approved these changes Feb 17, 2022
View reviewed changes
Copy link
Contributor

@arhimondr arhimondr left a comment

Choose a reason for hiding this comment

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

LGTM

core/trino-main/src/main/java/io/trino/execution/MemoryRevokingScheduler.java Outdated Show resolved Hide resolved
core/trino-main/src/main/java/io/trino/memory/ClusterMemoryManager.java Outdated Show resolved Hide resolved
core/trino-main/src/main/java/io/trino/memory/QueryContext.java Show resolved Hide resolved
@losipiuk losipiuk force-pushed the lo/drop-pools-concept branch from 5c61526 to cd2cdda Compare February 17, 2022 21:00
@losipiuk
Copy link
Member Author

@arhimondr AC: diff

@losipiuk
Copy link
Member Author

CI:#10932

@losipiuk losipiuk merged commit 698ff6c into trinodb:master Feb 18, 2022
@github-actions github-actions bot added this to the 372 milestone Feb 18, 2022
@mosabua mosabua mentioned this pull request Feb 22, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@electrum electrum electrum left review comments

@arhimondr arhimondr arhimondr approved these changes

@findepi findepi findepi approved these changes

@martint martint Awaiting requested review from martint

@dain dain Awaiting requested review from dain

@linzebing linzebing Awaiting requested review from linzebing

Assignees
No one assigned
Labels
cla-signed
Milestone
372
Development

Successfully merging this pull request may close these issues.

Remove reserved pool
5 participants
@losipiuk @electrum @dain @findepi @arhimondr