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

Load catalogs concurrently #15295

Merged

Conversation

findepi
Copy link
Member

@findepi findepi commented Dec 5, 2022

When plugin concurrent loading was added
(c5ac103), the catalogs could be loaded concurrently easily, because ConnectorManager.createCatalog was synchronized. The code has evolved since then, reducing contention significantly and thus allowing for parallel startup.

Fixes #15234
Follows #9521

@findepi findepi requested review from dain and losipiuk December 5, 2022 13:33
@cla-bot cla-bot bot added the cla-signed label Dec 5, 2022
@findepi findepi added the no-release-notes This pull request does not require release notes entry label Dec 5, 2022
@findepi
Copy link
Member Author

findepi commented Dec 5, 2022

Not sure whether we need release notes since it's an experimental feature (experimental.concurrent-startup).

cc @colebow

Copy link
Member

@losipiuk losipiuk left a comment

Choose a reason for hiding this comment

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

LGTM.

I am not 100% sure but I recall there was a discussion if parallel startup would not make it harder to debug startup issues. @kokosing ?

@findepi
Copy link
Member Author

findepi commented Dec 5, 2022

I am not 100% sure but I recall there was a discussion if parallel startup would not make it harder to debug startup issues. @kokosing ?

With ongoing work on Dynamic Catalogs (#12709), the concurrency must not be a problem, so it's actually good we're exercising this code more.
cc @dain

When plugin concurrent loading was added
(c5ac103), the catalogs could not be
loaded concurrently easily, because `ConnectorManager.createCatalog` was
synchronized. The code has evolved since then, reducing contention
significantly and thus allowing for parallel startup.
@findepi findepi force-pushed the findepi/load-catalogs-concurrently-55db57 branch from c9722ca to 92b128c Compare December 5, 2022 18:56
@findepi findepi merged commit 9cbf0f2 into trinodb:master Dec 5, 2022
@findepi findepi deleted the findepi/load-catalogs-concurrently-55db57 branch December 5, 2022 22:28
@github-actions github-actions bot added this to the 404 milestone Dec 6, 2022
@Jackhuihui
Copy link

@findepi How could I delete a Catalog in CoordinatorDynamicCatalogManager.java

@findepi
Copy link
Member Author

findepi commented Feb 28, 2023

i don't know, and it doesn't seem related to this PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-signed no-release-notes This pull request does not require release notes entry
Development

Successfully merging this pull request may close these issues.

Load catalogs concurrently
3 participants