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

Remove reserved pool #6677

Closed
sopel39 opened this issue Jan 21, 2021 · 5 comments · Fixed by #11071
Closed

Remove reserved pool #6677

sopel39 opened this issue Jan 21, 2021 · 5 comments · Fixed by #11071
Labels
maintenance Project maintenance task

Comments

@sopel39
Copy link
Member

sopel39 commented Jan 21, 2021

Discussion link: https://trinodb.slack.com/archives/CP1MUNEUX/p1610630362006000

@arhimondr
Copy link
Contributor

I assume after this refactor there should always be a single memory pool and it should be reflected at the code level. More specifically it would result in replacing Map<MemoryPoolId, ClusterMemoryPool> pools with simply ClusterMemoryPool memoryPool in multiple places in the codebase.

Is my understanding correct?

CC: @sopel39 @findepi @losipiuk @martint @dain

@findepi
Copy link
Member

findepi commented Feb 15, 2022

@arhimondr i am not entirely sure about this.
i am sure removing the reserved pool without this simplification is a good change.
i don't see any future use of >1 memory pools, but that doesn't mean there won't be.

i am personally ok either way, but please wait for opinion from others

@sopel39
Copy link
Member Author

sopel39 commented Feb 15, 2022

Could multiple memory pools potentially be used with resource groups in the future?

@martint
Copy link
Member

martint commented Feb 15, 2022

I don't think there's currently any use case for multiple pools, so if we're going to have just one, we should get rid of the infrastructure for managing multiple ones. We can always add that back if it ever becomes necessary.

@dain
Copy link
Member

dain commented Feb 15, 2022

I think we should get rid of supporting multiple pools, unless there is some reason someone to keep it right now. We can always add a new system if some comes up with a reason to segment usage in the future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance Project maintenance task
5 participants