-
Notifications
You must be signed in to change notification settings - Fork 915
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
[JNI] Adds retryCount to RmmEventHandler.onAllocFailure #11940
[JNI] Adds retryCount to RmmEventHandler.onAllocFailure #11940
Conversation
// this should not be called since it was the previous interface, | ||
// and it was abstract before. | ||
return false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since it should not be called, we may throw an exception instead, so we can know that there was something wrongly called and reached here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, I can make this throw. Note that this is going away very soon, it is mainly to give us a few days while we update the rest of the code. The end result is that we'll remove these default implementations, and callers must use the non-deprecated code.
// newer code should override this implementation of `onAllocFailure` to handle | ||
// the `isRetry` flag. Otherwise, we call the prior implementation to not | ||
// break existing code. | ||
return onAllocFailure(sizeRequested); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems to just call the function above (which returns false
all the time)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you have code that implemented onAllocFailure
(all existing code), no it just calls that impl. So this should be clearer given an exception in the deprecated code perhaps.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comment nit but otherwise lgtm.
Co-authored-by: Jason Lowe <[email protected]>
Codecov ReportBase: 87.40% // Head: 86.87% // Decreases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## branch-22.12 #11940 +/- ##
================================================
- Coverage 87.40% 86.87% -0.54%
================================================
Files 133 133
Lines 21833 21905 +72
================================================
- Hits 19084 19030 -54
- Misses 2749 2875 +126
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
@gpucibot merge |
This adds the method
boolean onAllocFailure(long sizeRequested, int retryCount)
toRmmEventHandler
, to help handling code keep track of the number of times an allocation failure has been retried.With this code callers can perform extra logic that depends on whether the callback was due to a brand new allocation failure, or one that has failed in the past and is being retried.
This will be used here: NVIDIA/spark-rapids#6768