Ignore the previous job_id inside fill_reservations() #141
Conversation
|
Hi @thinkharderdev, could you help review this PR? |
|
@yahoNanJing It looks like this was building on another PR that has now been merged. Could you rebase / upmerge so we can see the changes specific to this PR? |
|
Thanks @andygrove. Just rebased. I will follow the order listed in #129 to raise PRs for a few performance improvements. |
| if let Some(output_links) = | ||
| self.output_links.get_mut(&unresolved_shuffle.stage_id) | ||
| { | ||
| if !output_links.contains(&self.current_stage_id) { |
There was a problem hiding this comment.
Would it be better to make output_links a HashSet instead of a Vec, given that duplicate values are not allowed?
|
|
||
| if let Some(deps) = self.stage_dependencies.get_mut(&self.current_stage_id) { | ||
| deps.push(unresolved_shuffle.stage_id) | ||
| if !deps.contains(&unresolved_shuffle.stage_id) { |
There was a problem hiding this comment.
Same question here - would HashSet make more sense?
There was a problem hiding this comment.
For deps and output_links, there will not be many elements. I think that's why it uses vec rather than the set 🤣 . I'm OK to change it to be set.
There was a problem hiding this comment.
I don't have a strong opinion on this. Set would reduce the code complexity slightly but possibly add some overhead. 🤷♂️
Which issue does this PR close?
Closes #138.
Rationale for this change
What changes are included in this PR?
Are there any user-facing changes?