[SCB-1232] make GroupExecutor configuration compatible to old version #1161
Conversation
| "coreThreads is bigger than maxThreads, change from 25 to 10.", | ||
| collector.getEvents().get(collector.getEvents().size() - 2).getMessage()); | ||
| collector.teardown(); | ||
| Assert.assertEquals(10, groupExecutor.maxThreads); |
There was a problem hiding this comment.
Hello, in the UTs below, the groupExecutor.maxThreads is checked twice but the groupExecutor.maxThreads is not checked.
There was a problem hiding this comment.
done
rewrite the compatible rule:
1.maxThreads always be the max value of (oldMax, core, newMax), if still no value, then default to 100
2.if not configured queue, then coreThreads same to maxThreads
3.if configured queue, and queue size is not max int, then coreThreads default to be 25
There was a problem hiding this comment.
Why you keep this:
3.if configured queue, and queue size is not max int, then coreThreads default to be 25
I can't figure out the relationship between queue size and max thread size. For example, if users configure queue size to be MAX_INT/2
There was a problem hiding this comment.
because of jdk thread pool rule and compatible to old version
if upgrade from old version, no queueSize configured, it will be max int, if queueSize is too big, then coreThreads different to maxThreads is no meaning
but we can not know how big is "too big"
if developers configured queueSize， we can only treat the developer know what happened, just use the existing value.
| + "1.use core threads.\n" | ||
| + "2.if all core threads are busy, then queue the request.\n" | ||
| + "3.if queue is full, then create new thread util reach the limit of max threads.\n" | ||
| + "4.if queue is full, and threads count is max, then reject the request."); |
There was a problem hiding this comment.
Do we change the stratege to create new thread first until the limit of max threads is reached. And then queue the requests?
There was a problem hiding this comment.
yes, i have wrote the reason in the PR comment
and i will copy it to jira task comment
wujimin
force-pushed
the
group-executor-config-compatible
branch
from
9ba54eb
to
585edca
Compare
| coreThreads = maxThreads; | ||
| LOGGER.info("not configured {}, make coreThreads and maxThreads to be {}.", KEY_MAX_QUEUE_SIZE, maxThreads); | ||
| } else { | ||
| coreThreads = coreThreads <= 0 ? 25 : coreThreads; |
There was a problem hiding this comment.
This statement should be calculated before
maxThreads = Math.max(coreThreads, maxThreads);
There was a problem hiding this comment.
only developers configured a small maxThreads and big coreThreads will cause problem: ThreadPoolExecutor will throw exception in constructor
i think it's developers's duty to make a right configuration
we just handle compatible sense and default value.
expect to work like tomcat thread pool
but jdk standard thread pool's behavior is different:
to do what we want, need to lock the executor several times for one task, that's too bad
so only make compatible to old version temporary