Skip to content

Conversation

@Kobzol
Copy link
Member

@Kobzol Kobzol commented Oct 11, 2025

We should be checking the validity of the tasks on the server too, as doing it on the client can cause race conditions, in theory. But even if that happens, the server will reject the submit anyway, I suppose.

@Kobzol Kobzol force-pushed the submit-previous-job-dependency branch from 61e1188 to efb657c Compare October 11, 2025 14:00
@spirali
Copy link
Collaborator

spirali commented Oct 11, 2025

We should be checking the validity of the tasks on the server too, as doing it on the client can cause race conditions, in theory. But even if that happens, the server will reject the submit anyway, I suppose.

Yes, Tako checks this, so in the case race condition. the second submit gets an error.

I will do a proper code review tomorrow.

while idx < new_tasks.len() {
if let Some(consumers) = consumers.get(&new_tasks[idx].id) {

let mut handle_consumers =
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think that there is no need to construct "deps_from_previous_submit", you just need to check that tasks already exists (ID exits) and then forget it. You just need to modify the check for emptiness of deps. When all deps depends only on previously existing tasks, then you can put it immediately into new_tasks. Then there is no need to process anything like "deps_from_previous_submit" with handle_consumers.

(This is written from a hospital bed, so I may be wrong:)

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point, it's a nice simplification. It needed a few more changes, but it's better this way, I think.

@Kobzol Kobzol force-pushed the submit-previous-job-dependency branch from efb657c to 940e9ed Compare October 13, 2025 09:01
}

let task_deps_from_this_submit: Vec<JobTaskId> = t
.task_deps
Copy link
Member Author

Choose a reason for hiding this comment

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

Note: this could be

            .iter()
            .filter(|&task| !existing_tasks.contains(task))
            .copied()
            .collect();

But I think that pre-cloning might be better to avoid partial reallocations of the Vec, since the usual case is that all/most deps will be from the same submit.

@spirali spirali self-requested a review November 19, 2025 10:58
@spirali
Copy link
Collaborator

spirali commented Nov 19, 2025

Maybe we can avoid creating task_deps_from_this_submit at all. In the first run just to count the elements and then do the filtering again. But without benchmark it is just guessing. So LGTM for me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants