-
Notifications
You must be signed in to change notification settings - Fork 2k
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
core: fix blocked eval math #13104
core: fix blocked eval math #13104
Conversation
In the original test, the eval generator would use a random value for the job ID, resulting in an unxercised code path for duplicate blocked evals.
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.
Thanks for catching this and the code improvements! Hopefully it wasn't too bad to debug 😅
I pushed a commit to improve TestBlockedEvalsStats_BlockedResources
. It was using random job IDs, so it never exercised this code path...
} | ||
} | ||
|
||
// Copy returns a deep copy of the blocked resource stats. | ||
func (b BlockedResourcesStats) Copy() BlockedResourcesStats { | ||
func (b *BlockedResourcesStats) Copy() *BlockedResourcesStats { |
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.
Would it be worth adding a new nil
check on these methods since they are pointer receivers now?
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.
I considered this but ended up leaving it out; this object should never be nil. If it is I don't think there's any reasonable behavior other than to panic (although I guess we could do so with a better error message).
I think I got lucky in debugging this, one of the first log lines I added was to print the before and after values of the blocked resources even though it seems like simple math. Seeing the values change in the wrong direction was a big red flag. |
I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions. |
This PR fixes blocked eval resource math.
8506bd6 - the 1 line bug fix, where we now subtract off the correct eval
c3c739c - added tests around eval math
Fixes #12848
Backport to 1.3.x, 1.2.x