fix: CommandRunner metric has correct metric displayed when thread dies #4653
Conversation
stevenpyzhang
changed the title
stevenpyzhang
force-pushed
the
fix-command-runner-metric
branch
from
3a80fc6 to
6d5be45
Compare
stevenpyzhang
requested review from
rodesai and
aeroforero
and removed request for
aeroforero
| if (currentCommand == null) { | ||
| return CommandRunnerStatus.RUNNING; | ||
| return lastPollTime.get() == null || | ||
| Duration.between(lastPollTime.get(), clock.instant()).toMillis() | ||
| < NEW_CMDS_TIMEOUT.toMillis() * 3 | ||
| ? CommandRunnerStatus.RUNNING : CommandRunnerStatus.ERROR; |
There was a problem hiding this comment.
Can we just get rid of this and set the status to ERROR if we haven't polled in a while?
There was a problem hiding this comment.
We only start polling after we processPriorCommands so this could still be useful in the case of the CommandRunner getting stuck on start up.
stevenpyzhang
force-pushed
the
fix-command-runner-metric
branch
from
6d5be45 to
9251d03
Compare
stevenpyzhang
force-pushed
the
fix-command-runner-metric
branch
5 times, most recently
from
aef4638 to
310350d
Compare
| private final Duration commandRunnerHealthTimeout; | ||
| private final Clock clock; | ||
|
|
||
| public enum CommandRunnerStatus { | ||
| RUNNING, | ||
| STUCK, |
There was a problem hiding this comment.
why do we need this additional status? Once it's here we have to support it forever. Why is it not enough to know that the runner is up or down?
There was a problem hiding this comment.
I thought it would be helpful to be able to tell the difference between if the CommandRunner thread is stuck on a particular Command or the thread itself has died.
If we do force the server to shut down whenever the CommandRunner thread dies, then this additional status won't be needed.
stevenpyzhang
force-pushed
the
fix-command-runner-metric
branch
from
310350d to
b69ad12
Compare
stevenpyzhang
force-pushed
the
fix-command-runner-metric
branch
from
5cb1af0 to
11c0849
Compare
Description
Fixes: #4652
Add a new variable to CommandRunner that keeps track of the last time the command topic was polled. If there's too long of a duration between the current time when
CommandRunner.checkCommandRunnerStatusis called and the last poll, that means the thread is inERRORstate.Testing done
Killed the command runner thread and watched the metric. It transitioned to error state after 15 seconds.
Reviewer checklist