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.

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

Jump to bottom

Use retry with split in GpuCachedDoublePassWindowIterator #8476

Merged
merged 10 commits into from
Aug 31, 2023
Merged

Use retry with split in GpuCachedDoublePassWindowIterator #8476

merged 10 commits into from
Aug 31, 2023

Conversation

andygrove
Copy link
Contributor

Closes #8388

This PR updates GpuCachedDoublePassWindowIterator to use retry with splits for pre and post-processing.

@andygrove andygrove self-assigned this Jun 1, 2023
@andygrove andygrove added the reliability Features to improve reliability or bugs that severly impact the reliability of the plugin label Jun 1, 2023
@andygrove
Copy link
Contributor Author

build

@andygrove
Copy link
Contributor Author

build

@andygrove andygrove marked this pull request as ready for review June 2, 2023 13:41
revans2
revans2 reviewed Jun 2, 2023
View reviewed changes
Copy link
Collaborator

@revans2 revans2 left a comment

Choose a reason for hiding this comment

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

Sorry I didn't finish the review. Will look at it more soon once I fix the other places that we are trying to use withRetry in a way that is not going to close the Iterator that is returned.

sql-plugin/src/main/scala/com/nvidia/spark/rapids/GpuWindowExec.scala Show resolved Hide resolved
sql-plugin/src/main/scala/com/nvidia/spark/rapids/GpuWindowExec.scala Outdated
@@ -1694,6 +1696,12 @@ class GpuCachedDoublePassWindowIterator(

waitingForFirstPass.foreach(_.close())
waitingForFirstPass = None

firstPassIter.foreach(_.foreach(_._1.foreach(_.close())))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why are we pulling off of the results from the iterators to close them? This is not common unless the Iterators are backed by a cache. But then why have it be an Iterator instead of a List or ArrayBuffer or something like that.

Are we risking leaking data if the retry iterator is not fully drained? If so then there are a number of other bugs that we need to fix.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Okay I filed #8482 for this. Could you add a comment here about it so that we can find a better way to clean this up?

revans2
revans2 previously approved these changes Jun 2, 2023
View reviewed changes
Copy link
Collaborator

@revans2 revans2 left a comment

Choose a reason for hiding this comment

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

I think this code is okay, but I think we could make it much cleaner if we had a better way to close the iterators and divide things up into sub-iterators. Perhaps it would be better to wait until 23.08 and we have fixed the leak that you are worried about.

@andygrove andygrove marked this pull request as draft June 2, 2023 20:49
@andygrove
Copy link
Contributor Author

I think this code is okay, but I think we could make it much cleaner if we had a better way to close the iterators and divide things up into sub-iterators. Perhaps it would be better to wait until 23.08 and we have fixed the leak that you are worried about.

Sounds good. I have moved this to draft for now.

@andygrove andygrove changed the base branch from branch-23.06 to branch-23.08 June 6, 2023 01:48
@andygrove andygrove dismissed revans2’s stale review June 6, 2023 01:48

The base branch was changed.

@andygrove andygrove changed the base branch from branch-23.08 to branch-23.10 August 11, 2023 00:34
@andygrove andygrove marked this pull request as ready for review August 29, 2023 16:06
@andygrove
Copy link
Contributor Author

build

@andygrove andygrove merged commit bdbba63 into NVIDIA:branch-23.10 Aug 31, 2023
27 checks passed
@andygrove andygrove deleted the double-pass-iter branch August 31, 2023 00:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@revans2 revans2 revans2 approved these changes

@abellina abellina Awaiting requested review from abellina

Assignees

@andygrove andygrove

Labels
reliability Features to improve reliability or bugs that severly impact the reliability of the plugin
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[FEA] Update GpuCachedDoublePassWindowIterator to use RetryWithSplit
2 participants
@andygrove @revans2