Skip to content

NO-JIRA: task graph: test speedup and code cleaup #1101

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.

Sign up for GitHub

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

Merged
openshift-merge-bot merged 2 commits into from
Nov 5, 2024
Merged

NO-JIRA: task graph: test speedup and code cleaup #1101

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1c71a50
task graph test: do not wait after cancelation
petr-muller Oct 30, 2024
df39a81
task graph: simplify code
petr-muller Oct 30, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions pkg/payload/task_graph.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -514,9 +514,8 @@ func RunGraph(ctx context.Context, graph *TaskGraph, maxParallelism int, fn func
inflight--
case <-ctx.Done():
select {
case runTask := <-workCh: // workers canceled, so remove any work from the queue ourselves
case <-workCh: // workers canceled, so remove any work from the queue ourselves
Copy link
Member

@hongkailiu hongkailiu Nov 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NIT:

With submitted[runTask.index] = false removed, perhaps we can update the comment as well?

Suggested change
case <-workCh: // workers canceled, so remove any work from the queue ourselves
case <-workCh: // Reset the inflight counter as workers got canceled. We never submit more work once canceled and thus no need to reset the submitted records.

Copy link
Member Author

@petr-muller petr-muller Nov 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The important part is still the "remove any work from the queue" though - that needs to stay. The channel gets drained (eventually...).

I'm not convinced about the usefulness of mentioning the submitted records - it explains the absence of code, but that only makes sense attached to a commit/code change, does not make that much sense in the actual code? "We are not doing something because it does not need to be done"-like comment IMO makes sense only if that absence is actually surprising, which does not seem to be the case here?

Copy link
Member

@hongkailiu hongkailiu Nov 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the "remove any work from the queue" though - that needs to stay.

I think i got it now.
"remove any work from the queue ourselves" corresponds to <-workCh (that is why they stay in the same line).

My mistake was I thought the comment was for the code below, currently having only inflight-- which i couldn't relate to the comment. Actually, inflight-- is the additional things we need to do when <-workCh takes place.

We are not doing something because it does not need to be done

I got the point.

like comment IMO makes sense only if that absence is actually surprising,

It would probably surprise the author of "submitted[runTask.index] = false". 😉 When we push a task, we do submitted[nextNode] = true, my intuitive feeling is that we would set it to false when it is removed from the queue before the task is completed. With a comment, it would be telling that we do not do it intentionally, instead of forgetting it. Maybe it is just me. Just a NIT anyway.

Copy link
Member Author

@petr-muller petr-muller Nov 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would probably surprise the author of "submitted[runTask.index] = false"

😁

Copy link
Member

@wking wking Nov 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm the author of that line, in 632e763 (#455), and Petr's "We never submit more work once canceled..." makes sense to me, and if I git blame ... the line with the comment, I'll find his commit message, so all good on that side :)

inflight--
submitted[runTask.index] = false
default:
}
}
Expand Down
1 change: 0 additions & 1 deletion pkg/payload/task_graph_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -949,7 +949,6 @@ func TestRunGraph(t *testing.T) {
callbacks: map[string]callbackFn{
"a": func(t *testing.T, name string, ctx context.Context, cancelFn func()) error {
cancelFn()
time.Sleep(time.Second)
Copy link
Member

@wking wking Nov 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd added this with the test-case back in eaa3d19 (#255). I didn't talk about the Sleep specifically in the commit message, but my guess is that what I was aiming for was "make sure b isn't running because the task-graph runner knows a failed" to avoid "b would have run, except the runner got lucky and raced closed before it got around to completing". It's been in place for a long time now though, and I'm fine dropping the "be-extra certain" sleep at this point in order to allow us to run tests more quickly.

return nil
},
"*": func(t *testing.T, name string, ctx context.Context, cancelFn func()) error {
Expand Down