-
Notifications
You must be signed in to change notification settings - Fork 90
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
IGNITE-22118 Explicitly specify pool to execute start/stop on IgniteComponent #3784
Conversation
* @return Future that will be completed when the asynchronous part of the start is processed. | ||
*/ | ||
CompletableFuture<Void> startAsync(); | ||
CompletableFuture<Void> startAsync(ExecutorService startupExecutor); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest to pass something like ComponentContext instead of executor, which should provides executorservice and whatever we want later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
return updateLog.startAsync() | ||
.thenCompose(none -> { | ||
return updateLog.startAsync(startupExecutor) | ||
.thenComposeAsync(none -> { | ||
if (latestCatalogVersion() == emptyCatalog.version()) { | ||
int initializedCatalogVersion = emptyCatalog.version() + 1; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Which thread will complete catalogReadyFuture?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Explicitly set to the startupExecutor pool
* | ||
* @return Future that will be completed when the asynchronous part of the stop is processed. | ||
*/ | ||
default CompletableFuture<Void> stopAsync() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's required for micronaut annotations on TestFactory to work.
If we are not okay with this method I can spend some time rewriting tests instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's either rewrite the tests or mark the method as TestOnly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
marked as TestOnly
https://issues.apache.org/jira/browse/IGNITE-22118
Added
ExecutorService
toIgniteComponent.startAsync
andIgniteComponent.stopAsync
. Any async part of start/stop is expected to run in the corresponding thread pool.This PR is mostly the change of method signature, only a few components had their startAsync methods changed.
The ones that were changed: