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 plugins concurrently #9521

Merged
merged 1 commit into from
Oct 12, 2021
Merged

Conversation

findepi
Copy link
Member

@findepi findepi commented Oct 5, 2021

This adds an opt-in capability to start server concurrent. When enabled,
plugins are loaded concurrently.

In local testing, this improves DevelopmentServer startup time from
about 88s to about 38s (actual times very much depend on multiple
factors).

As a follow-up work, catalogs can be made to load concurrently.

@kokosing
Copy link
Member

kokosing commented Oct 6, 2021

In local testing, this changes DevelopmentServer startup time from about 62s to about 33s.

Is it possible that having only 2 threads would get same results, so having Runtime.getAvailableProcessors() is too much. Do you know where where we spend time? Is it IO (class loading)?

@@ -2,6 +2,7 @@ node.id=will-be-overwritten
node.environment=test

coordinator=true
experimental.concurrent-startup=true
Copy link
Member

Choose a reason for hiding this comment

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

How faster it become in produce tests?

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 didn't measure this.

@findepi
Copy link
Member Author

findepi commented Oct 6, 2021

Is it possible that having only 2 threads would get same results,

it is. I would prefer to leave fine-tunings and for the future.
I want to focus on the basic fact that there is quite some work to be done during startup, and the work is parallelizable.
For plugin loading (poking into jars to uncompress set of initially required classes) maximum parallelism possible may depend on CPU vs disk, so my local results may not match exactly yours.

@wendigo
Copy link
Contributor

wendigo commented Oct 6, 2021

@findepi I'm worried about logging during startup. Are logs intertwined right now?

@findepi
Copy link
Member Author

findepi commented Oct 6, 2021

Are logs intertwined right now?

absolutely.

@wendigo
Copy link
Contributor

wendigo commented Oct 6, 2021

@findepi Maybe we could defer dumping configuration after plugin/catalogs are loaded and server is started? I think that flushing output takes some portion of that 30s. Alternatively we could buffer logs and flush them at once after catalog is loaded. WDYT?

@wendigo
Copy link
Contributor

wendigo commented Oct 6, 2021

presto-master       | 2021-10-06T13:50:01.349+0545	ERROR	main	io.trino.server.Server	java.io.FileNotFoundException: etc/catalog/kafka (Is a directory)
presto-master       | java.lang.RuntimeException: java.io.FileNotFoundException: etc/catalog/kafka (Is a directory)
presto-master       | 	at io.airlift.concurrent.MoreFutures.getFutureValue(MoreFutures.java:202)
presto-master       | 	at io.airlift.concurrent.MoreFutures.getFutureValue(MoreFutures.java:176)
presto-master       | 	at io.airlift.concurrent.MoreFutures.getDone(MoreFutures.java:278)
presto-master       | 	at io.trino.util.Executors.executeUntilFailure(Executors.java:40)
presto-master       | 	at io.trino.metadata.StaticCatalogStore.loadCatalogs(StaticCatalogStore.java:74)
presto-master       | 	at io.trino.server.Server.doStart(Server.java:125)
presto-master       | 	at io.trino.server.Server.lambda$start$0(Server.java:78)
presto-master       | 	at io.trino.$gen.Trino_fd69855____20211006_080434_1.run(Unknown Source)
presto-master       | 	at io.trino.server.Server.start(Server.java:78)
presto-master       | 	at io.trino.server.TrinoServer.main(TrinoServer.java:38)
presto-master       | Caused by: java.io.FileNotFoundException: etc/catalog/kafka (Is a directory)

@findepi
Copy link
Member Author

findepi commented Oct 6, 2021

I think that flushing output takes some portion of that 30s. Alternatively we could buffer logs and flush them at once after catalog is loaded. WDYT?

there is not a ton of logs, so logging overhead is negligible.
buffering logs may have the inadvertent effect that logs are not available at the point when one needs them

@findepi findepi force-pushed the concurrent-startup branch 2 times, most recently from 73b0152 to 933c80e Compare October 11, 2021 12:03
@findepi findepi changed the title Start server concurrently Load plugins concurrently Oct 11, 2021
@findepi findepi requested a review from kokosing October 11, 2021 12:03
@findepi
Copy link
Member Author

findepi commented Oct 11, 2021

per conversation with @kokosing i dropped catalog parallel loading.

Also @martint told me he has a more elaborate version of catalog parallel loading.
My version wasn't very concurrent, since io.trino.connector.ConnectorManager#createCatalog(java.lang.String, java.lang.String, java.util.Map<java.lang.String,java.lang.String>) is synchronized.

@kokosing ptal

This adds an opt-in capability to start server concurrent. When enabled,
plugins are loaded concurrently.

In local testing, this improves `DevelopmentServer` startup time from
about 88s to about 38s (actual times very much depend on multiple
factors).

As a follow-up work, catalogs can be made to load concurrently.
@findepi findepi merged commit c5ac103 into trinodb:master Oct 12, 2021
@findepi findepi deleted the concurrent-startup branch October 12, 2021 09:56
@github-actions github-actions bot added this to the 364 milestone Oct 12, 2021
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.

3 participants