[Fix-16382] Fix the bug of async master task casthread pool invocations ramp-up #16461
Conversation
Dyqer
requested review from
caishunfeng,
SbloodyS and
ruanwenjun
as code owners
| } | ||
|
|
||
| @Override | ||
| public long getDelay(TimeUnit unit) { | ||
| long nextExecuteTimeDelay = Math.min(currentStartTime + executeInterval, timeout) - System.currentTimeMillis(); | ||
| return unit.convert(nextExecuteTimeDelay, TimeUnit.MILLISECONDS); | ||
| return unit.convert(this.executeInterval, TimeUnit.MILLISECONDS); |
There was a problem hiding this comment.
Have you test this? Once the element changed in the delay queue, the delay time will recalculate, in this pr the delay time will be a fixed value, this will cause some function will never be triggered.
There was a problem hiding this comment.
sorry for that, I changed it.
| public synchronized void refreshStartTime() { | ||
| if (executeTimes != 0) { | ||
| // The first time doesn't have delay | ||
| executeTimes++; | ||
| } else { | ||
| currentStartTime = System.currentTimeMillis(); | ||
| } | ||
| executeTimes++; |
There was a problem hiding this comment.
The executeTimes is only a mark flag, it is not used to represent execute the real times.
There was a problem hiding this comment.
we can record the execute times for monitoring
There was a problem hiding this comment.
yes, it's great, we can add this in task metrics.
|
The key problem related to your issue is the async task doesn't support timeout, the async should stop once it's timeout. |
Dyqer
force-pushed
the
fix-async-master-task
branch
from
98d7770
to
0c3b85d
Compare
but the timeout observer is in master standby list, it may have 1~10 minutes later than real timeout, the functions will be invoked in a dead loop during those 10 minutes. |
@ruanwenjun
I think the best solution is to ignore the timeout logic here and only focus on the execution interval. The timeout event should still be handled by the stateEventhandler. |
Yes, there exist a bug as you mention, and you are right, the task executor don't need to care about the timeout, all timeout event should be controlled by master event engine. |
There was a problem hiding this comment.
LGTM, and we didn't have integration test for the logic task, after #16327, we can easily add this kind of case.
|
@ruanwenjun @SbloodyS anyone can help merge? Seems github actions stuck |
CI failed. You should run |
Dyqer
force-pushed
the
fix-async-master-task
branch
from
3d0f3df
to
aebfea6
Compare
Oh, Thank you!, please help to review again @ruanwenjun @SbloodyS |
|
Purpose of the pull request
fix #16460
Brief change log
Verify this pull request
This pull request is code cleanup without any test coverage.
(or)
This pull request is already covered by existing tests, such as (please describe tests).
(or)
This change added tests and can be verified as follows:
(or)
Pull Request Notice
Pull Request Notice
If your pull request contain incompatible change, you should also add it to
docs/docs/en/guide/upgrede/incompatible.md