APP-10065: Record job completion times; include in GetMachineStatus #5493
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
viambot
added
the
safe to test
aldenh-viam
force-pushed
the
jobhistory
branch
from
fb80ee0 to
92b2c66
Compare
viambot
added
safe to test
aldenh-viam
force-pushed
the
jobhistory
branch
from
92b2c66 to
158f3bd
Compare
viambot
added
safe to test
aldenh-viam
force-pushed
the
jobhistory
branch
from
158f3bd to
6fe1130
Compare
viambot
added
safe to test
aldenh-viam
force-pushed
the
jobhistory
branch
from
6fe1130 to
63cefa1
Compare
viambot
added
safe to test
aldenh-viam
force-pushed
the
jobhistory
branch
from
63cefa1 to
d35568a
Compare
viambot
added
safe to test
aldenh-viam
force-pushed
the
jobhistory
branch
from
d35568a to
367ce5b
Compare
viambot
added
safe to test
aldenh-viam
force-pushed
the
jobhistory
branch
from
367ce5b to
80ff6ad
Compare
viambot
added
safe to test
aldenh-viam
force-pushed
the
jobhistory
branch
from
80ff6ad to
ba78f56
Compare
viambot
added
safe to test
aldenh-viam
force-pushed
the
jobhistory
branch
from
ba78f56 to
3d53de9
Compare
viambot
added
safe to test
aldenh-viam
force-pushed
the
jobhistory
branch
from
1275925 to
e076c4f
Compare
viambot
added
safe to test
aldenh-viam
force-pushed
the
jobhistory
branch
from
e076c4f to
ca0501d
Compare
viambot
added
safe to test
aldenh-viam
force-pushed
the
jobhistory
branch
from
a575750 to
7a6c5fd
Compare
viambot
added
safe to test
aldenh-viam
force-pushed
the
jobhistory
branch
from
7a6c5fd to
4b4d65d
Compare
viambot
added
safe to test
…remove JobManager() from robot interface
aldenh-viam
force-pushed
the
jobhistory
branch
from
4b4d65d to
503e408
Compare
viambot
added
safe to test
cheukt
reviewed
Nov 24, 2025
Member
cheukt
left a comment
cheukt
left a comment
There was a problem hiding this comment.
generally LGTM, just a q around whether testing is complete
robot/impl/jobmanager_test.go
Outdated
| test.That(tb, len(failJob.RecentSuccessfulRuns), test.ShouldEqual, 0) | ||
| test.That(tb, len(failJob.RecentFailedRuns), test.ShouldBeGreaterThan, 0) | ||
| }) | ||
| // TODO: test flaky job with both successes and panics |
Member
There was a problem hiding this comment.
are you adding another test here?
Contributor
Author
There was a problem hiding this comment.
Originally wasn't going to, as I don't think it adds much additional value (TODO msg just for completeness), but I don't mind quickly adding one.
viambot
added
safe to test
aldenh-viam
force-pushed
the
jobhistory
branch
from
690d285 to
93f3154
Compare
viambot
added
safe to test
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Basic implementation using an
ssync.MapofJobName -> 2x "container/ring".Lock contention is a potential issue, as
GetMachineStatusis called every second by each App tab open. It will be most noticeable on busy robots (with many App tabs open) that run frequent jobs (short-lived continuous and/or high count).Do we think it's worth optimizing further for this case?