-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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-7993 Striped pool can't be disabled #3661
Conversation
*/ | ||
public class StripedExecutorProxy extends StripedExecutor { | ||
/** Target executor service */ | ||
private volatile ExecutorService executor; |
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 is not this final?
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 is not final because the original constructor wasn't overridden here: StripedExecutorProxy(int cnt, String igniteInstanceName, String poolName, final IgniteLogger log, boolean stealTasks)
I can override it and mark the field as final.
* @throws IgniteException If executor is null. | ||
*/ | ||
private void checkExecutor() { | ||
if (executor == null) |
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 cannot find code that turns executor to null.
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.
As I mentioned above, there is can be a case when StripedExecutorProxy is created using original constructor (without executor parameter).
cfg.getIgniteInstanceName(), | ||
"sys", | ||
log); | ||
if (cfg.getStripedPoolSize() > 0) { |
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 don't like this solution. I would like to change all usages of striped executor to a logic that will choose between system and striped pools depending on striped pool enabled or not.
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 agree. What if we add a little bit more changes?
-
Rename StripedExecutor to StripedExecutorImpl and add an interface named StripedExecutor. Currently, StripedExecutor class implements only ExecutorService interface.
-
Add new flag (system property) IGNITE_STRIPED_POOL_DISABLED (false by default).
-
Update StripedPoolProxy "extends ThreadPoolExecutor implements StripedPool".
-
Restore validateThreadPoolSize(cfg.getStripedPoolSize())
-
If parameter IGNITE_STRIPED_POOL_DISABLED is "true" then where will be used StripedPoolProxy instead of StripedPool.
This way we will have a sort of second system pool (classic ThreadPoolExecutor) with StripedExecutor interface.
@yzhdanov, how does it sound to you?
* Renamed StripedExecutor to StripedExecutorImpl * Added StripedExecutor interface * Update StripedExecutorProxy (extends IgniteThreadPoolExecutor impl StripedExecutor)
Restored the capability to disable striped pool