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

Safely close multiple resources in RapidsBufferCatalog #11307

Merged
merged 3 commits into from
Aug 9, 2024

Conversation

jihoonson
Copy link
Collaborator

@jihoonson jihoonson commented Aug 7, 2024

Minor improvement to safely close all resources in RapidsBufferCatalog even if one of them fails to close.

Contributes to #10502

Signed-off-by: Jihoon Son <ghoonson@gmail.com>
gerashegalov
gerashegalov previously approved these changes Aug 8, 2024
@jihoonson
Copy link
Collaborator Author

Build

@jihoonson
Copy link
Collaborator Author

Can someone help to run the CI? I don't seem to have enough power for that.

@gerashegalov
Copy link
Collaborator

build

@jihoonson
Copy link
Collaborator Author

Looking into the CI failure. Seems like a legit issue.

@jihoonson
Copy link
Collaborator Author

Looking into the CI failure. Seems like a legit issue.

It was because of the missing nullifying. Fixed in 6607bbb. As part of this, I removed the AutoCloseable workaround that only sets memoryEventHandler to null, hoping that this would have no impact. It is a new behavior that RapidsBufferCatalog closes all its resources and then nullifies them. The previous behavior was to close and nullify them one by one.

@jihoonson jihoonson added the bug Something isn't working label Aug 8, 2024
@jihoonson
Copy link
Collaborator Author

build

Copy link
Collaborator

@gerashegalov gerashegalov left a comment

Choose a reason for hiding this comment

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

LGTM

@jihoonson jihoonson merged commit 05152f7 into NVIDIA:branch-24.10 Aug 9, 2024
43 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants