Update set up for ScheduledExecutorService. #1705
Conversation
| return DgsDataLoaderProvider(applicationContext, dataloaderOptionProvider) | ||
| @ConditionalOnMissingBean | ||
| open fun scheduledExecutorService(): ScheduledExecutorService { | ||
| return Executors.newSingleThreadScheduledExecutor() |
There was a problem hiding this comment.
I think this will cause problems if someone is trying to autowire an Executor bean, since ScheduledExecutorService also implements Executor.
There was a problem hiding this comment.
Thanks, yeah I am changing it to add a quaifier for dgsScheduledExecutorService. Would that mitigate the issue you think?
There was a problem hiding this comment.
Yeah, a Qualifier would avoid the problem
| @@ -154,8 +156,14 @@ open class DgsAutoConfiguration( | |||
| } | |||
|
|
|||
| @Bean | |||
There was a problem hiding this comment.
Should we set destroyMethod="shutdown"?
srinivasankavitha
force-pushed
the
dataloader-improvements
branch
from
d979373
to
7ea60e3
Compare
| @@ -45,7 +48,9 @@ import kotlin.system.measureTimeMillis | |||
| */ | |||
| class DgsDataLoaderProvider( | |||
| private val applicationContext: ApplicationContext, | |||
| private val dataLoaderOptionsProvider: DgsDataLoaderOptionsProvider = DefaultDataLoaderOptionsProvider() | |||
| private val dataLoaderOptionsProvider: DgsDataLoaderOptionsProvider = DefaultDataLoaderOptionsProvider(), | |||
| private val scheduledExecutorService: ScheduledExecutorService = Executors.newSingleThreadScheduledExecutor(), | |||
There was a problem hiding this comment.
I don't really know when/why to use singleThreadScheduleExecutor vs a pool of scheduled threads with Executors.newScheduledThreadPool(int corePoolSize). It might be good to have this configurable, but I'm not sure.
There was a problem hiding this comment.
Since this is set up as a bean in our autoconfig, I'm thinking the the user can override the dgsScheduledExecutorService bean to set it up differently?
There was a problem hiding this comment.
I also thought about that and am not sure what a good default is. It probably depends on the load / RPS. If you have a lot of events scheduled at or around the same instant I can see one thread becoming a bottleneck; but I also think it's something that could be solved in a follow up.
There was a problem hiding this comment.
Thanks for the input folks! The default is just based on what the java-dataloader library already picked. Will merge this PR for now and we can make it configurable if needed in a follow up.
There was a problem hiding this comment.
Right, but the default uses a new executor for every registry created. That's in and of itself not ideal and runs into its own scaling problem of spinning up too many threads, but it wouldn't run into this specific scaling problem where an executor is shared since it's essentially spinning up a thread per request. I suspect a default like corePoolSize set to the number of CPUs might make more sense. But if the default here is a problem we'll see it in testing, and can fix it in a follow up like you said.
srinivasankavitha
force-pushed
the
dataloader-improvements
branch
from
b372218
to
01d2849
Compare
| @@ -118,7 +119,12 @@ object BaseDgsQueryExecutor { | |||
| .extensions(extensions.orEmpty()) | |||
| .build() | |||
| graphQLContextFuture.complete(executionInput.graphQLContext) | |||
| graphQL.executeAsync(executionInput) | |||
| graphQL.executeAsync(executionInput).thenApply { | |||
There was a problem hiding this comment.
whenComplete might be safer, although I don't know under what circumstances executeAsync ever really fails; maybe cancellation?
Add dgs qualifier for scheduled executor service. Update to java-dataloader 3.2.2 containing a fix for memory leak. Close the scheduled dataloader registry upon request completion. Use whenComplete instead of Apply for closing registry. Fix linter errors.
srinivasankavitha
force-pushed
the
dataloader-improvements
branch
from
2d8e832
to
2c5de10
Compare
This PR has the following changes: