-
Notifications
You must be signed in to change notification settings - Fork 232
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 support to RowToColumnarIterator #9066
Conversation
Signed-off-by: Firestarman <firestarmanllc@gmail.com>
build |
Signed-off-by: Firestarman <firestarmanllc@gmail.com>
build |
try { | ||
b.close(); | ||
} catch (Throwable e) { | ||
/* ignore the exception */ |
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, and the next catch seem a bit odd. What exception were you seeing?
Should this instead use something like safeClose()
(we only have that in scala right now) and produce a single exception after trying to close all, and returning a single exception object that has the individual exceptions supressed?
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.
I just want to make sure it will always try to close the HostColumns no matter if any exception happens when closing the builders. Something likes
try {
for (ai.rapids.cudf.HostColumnVector.ColumnBuilder b: builders) {
if (b != null) {
b.close();
}
}
} finally {
if (hostColumns != null) {
for (ai.rapids.cudf.HostColumnVector hcv: hostColumns) {
if (hcv != null) {
hcv.close();
}
}
hostColumns = null;
}
}
Is this OK ? @abellina @revans2, or do you want me to do as the following. Java requires explicit declaration for non runtime exceptions, so we only need to catch the runtime exceptions here.
RuntimeException toThrow = null;
for (ai.rapids.cudf.HostColumnVector.ColumnBuilder b: builders) {
if (b != null) {
try {
b.close();
} catch (RuntimeException e) {
if (toThrow == null) {
toThrow = e;
} else {
toThrow.addSuppressed(e);
}
}
}
}
if (hostColumns != null) {
for (ai.rapids.cudf.HostColumnVector hcv: hostColumns) {
if (hcv != null) {
try {
hcv.close();
} catch (RuntimeException e) {
if (toThrow == null) {
toThrow = e;
} else {
toThrow.addSuppressed(e);
}
}
}
}
hostColumns = null;
}
if (toThrow != null) {
throw toThrow;
}
}
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.
Picked the first one. If you prefer to the second option, I will update it accordingly.
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.
I agree with @abellina I am not a fan of throwing away an exception, especially a generic Throwable.
Signed-off-by: Firestarman <firestarmanllc@gmail.com>
build |
close #8350
This PR adds in the retry support to RowToColumnarIterator.
This PR adds a new API named
tryBuild
in theGpuColumnarBatchBuilder
class to support the retry mechanism.Builders can build the columns only once. So this change holds the host columns built from the builders and reuses the host columns to build the columnar batch as many times as it needs.