Skip to content
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

Add retry to BatchByKeyIterator #8058

Merged
merged 4 commits into from
Apr 19, 2023

Conversation

revans2
Copy link
Collaborator

@revans2 revans2 commented Apr 7, 2023

This fixes #7778

It also fixes a bug in closeOnExcept where scala can throw an exception for flow control which in my case resulted in a use after free error for an intermediate version of the patch.

Signed-off-by: Robert (Bobby) Evans <[email protected]>
@revans2 revans2 self-assigned this Apr 7, 2023
@revans2
Copy link
Collaborator Author

revans2 commented Apr 7, 2023

build

abellina
abellina previously approved these changes Apr 7, 2023
Copy link
Collaborator

@abellina abellina left a comment

Choose a reason for hiding this comment

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

minor comments.

spillableBuffers.append(pending.dequeue())
}
pendingSize = 0
last.foreach { lastSpill =>
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit, could be replaced with: spillableBuffers.appendAll(last). I didn't know that you could, but it looks like appendAll can handle an Option and if set to None it is a noop.

@revans2
Copy link
Collaborator Author

revans2 commented Apr 7, 2023

build

@sameerz sameerz added the reliability Features to improve reliability or bugs that severly impact the reliability of the plugin label Apr 8, 2023
@revans2
Copy link
Collaborator Author

revans2 commented Apr 13, 2023

@abellina could you take another look?

abellina
abellina previously approved these changes Apr 13, 2023
@revans2
Copy link
Collaborator Author

revans2 commented Apr 18, 2023

build

@revans2
Copy link
Collaborator Author

revans2 commented Apr 18, 2023

@abellina could you look at this again. I had to upmerge

@revans2 revans2 merged commit d0010f4 into NVIDIA:branch-23.06 Apr 19, 2023
@revans2 revans2 deleted the batch_by_key_retry branch April 19, 2023 13:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
reliability Features to improve reliability or bugs that severly impact the reliability of the plugin
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FEA] Handle SplitAndRetryOOM for BatchedByKey goal in GpuCoalesceBatches
3 participants