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
Update OptaplannerKieServerExtension.java #1875
Conversation
Can one of the admins verify this PR? Comment with 'ok to test' to start the build. |
.append(", maximumPoolSize = ") | ||
.append(this.threadPool.getMaximumPoolSize()); | ||
|
||
logger.info(s.toString()); |
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.
This is too verbose to output on info logging. Let's make this debug logging.
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.
No need to use StringBuilder here, it's equally fast as just String concatenation because there is no loop. (faster even, see shipilev's blog).
Furthermore, better to use this to avoid string concatenation if the logging level > debug.
logger.debug("OptaPlanner extension created with corePoolSize ({}), queueSize ({}), ...", corePoolSize, queueSize, ...)
ok to test |
@MusaTalluzi if you could take some time to review and test this changes to approve the PR, then let me know and I 'll merge it. Relates to https://issues.jboss.org/browse/BAPL-1364 - but I am now starting to doubt if it fixes it. I haven't investigated reqs in detail at this point - the bapl reopening relates to openshift. |
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.
See my earlier comments.
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.
Hey @robertonav20, welcome to KIE group! Please describe what problem you're trying to solve with this PR. Either try to explain here in PR description/comments or share links if the issue is already reported on StackOverflow or JIRA.
Hey yurloc, Below the old logic this.threadPool = new ThreadPoolExecutor( |
@robertonav20 What is the latest KIE Server version that is affected by this issue? Could you please try with 7.24.0.Final or newer? 7.24.0.Final was released shortly before you opened this PR and contains a fix for https://issues.jboss.org/browse/PLANNER-1553, which might also address your issue. |
@robertonav20 I just tried 7.24.0.Final and was I able submit problems to 30 solvers on my machine with 8 CPUs when I used 8 CPUs => 6 threads in the pool. This means maximum of 6 solvers solving in parallel. Other solving jobs will wait in the queue until solving requests exceed queue size. The default queue size is equal to the pool size so with N CPUs you can have N problems being solved and another N waiting in the queue. Any additional requests will be rejected until there's free space in the queue. If you want to be able to handle more than N+N request (allow more requests to wait in the queue instead of being rejected), use Let me know if that helps. |
Hi yurloc, So I update the version of kie server then use that system property right?! Thanks yurloc and tkobayash for fix! |
@robertonav20 Yes. Please upgrade to latest (at least 7.24.0.Final) and use the property to increase thread pool queue size. If this approach doesn't help, please file a JIRA ticket and provide more information (stack trace from server log, log line with thread pool information ( Closing this PR as the issue seems to be fixed by #1862 and also for the reason that merging this PR would revert #1862, introducing a regression. |
No description provided.