-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
solver: simplify edge-related functions in the solver #5401
Conversation
bb4d376
to
b67653b
Compare
func (e *edge) processUpdates(updates []pipe.Receiver[*edgeRequest, any]) { | ||
depChanged := false | ||
for _, upt := range updates { | ||
depChanged = e.processUpdate(upt) || depChanged |
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.
This is different behvior that captures last return value.
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.
This is likely the cause of the test failure then but I did not realize an or operation in Go could short circuit right to left. I figured it was the same as most languages where this was always evaluated left to right.
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.
Ah, I looked over the ||
first time and just saw the variable setting happened on each iteration. This looks correct to me for return value, but it does exec true = true
in a loop if the first item happens to be return true.
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.
Figured out what the original issue was. I misread a condition in creating the slow cache request function. I thought it was e.preprocessFunc(dep) != nil && e.slowCacheFunc(dep) != nil
but it was an ||
instead.
I believe this logic is correct. I can change it to be something like:
if e.processUpdate(upt) {
depChanged = true
}
But I thought the extra brevity may be easier to understand. Not a substantial difference between the two though.
b88d061
to
c3a67d4
Compare
Many of the functions in the solver that operated on the edges were very large which obscured their usage and the overall flow of the scheduler. This change refactors those functions into separate smaller functions to make it easier to follow the overall flow of the scheduler. Signed-off-by: Jonathan A. Sternberg <[email protected]>
1ef471b
to
37b6f87
Compare
Many of the functions in the solver that operated on the edges were very large which obscured their usage and the overall flow of the scheduler. This change refactors those functions into separate smaller functions to make it easier to follow the overall flow of the scheduler.