Github user sudheeshkatkam commented on a diff in the pull request:
https://github.com/apache/drill/pull/838#discussion_r117590009
--- Diff:
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/ScanBatch.java
---
@@ -173,9 +174,8 @@ public IterOutcome next() {
currentReader.allocate(mutator.fieldVectorMap());
} catch (OutOfMemoryException e) {
- logger.debug("Caught Out of Memory Exception", e);
clearFieldVectorMap();
- return IterOutcome.OUT_OF_MEMORY;
+ throw UserException.memoryError(e).build(logger);
--- End diff --
I am not sure if this specific line change is required, so please correct
me if I am wrong. Thinking out loud..
There are three places in ScanBatch where OutOfMemoryException is handled.
Since OutOfMemoryException is an unchecked exception, I could not quickly find
all the calls which trigger the exception in this method.
The [first
case](https://github.com/apache/drill/blob/master/exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/ScanBatch.java#L175)
and [second
case](https://github.com/apache/drill/blob/master/exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/ScanBatch.java#L215)
are similar in that `reader.allocate(...)` fails. So although there is no
unwind logic, seems to me, this case is correctly handled as no records have
been read, and so there is no need to unwind. Say this triggers spilling in
sort, then the query could complete successfully, if allocate succeeds next
time (and so on). Am I following this logic correctly?
But this does not seems to be case, as
[TestOutOfMemoryOutcome](https://github.com/apache/drill/blob/master/exec/java-exec/src/test/java/org/apache/drill/TestOutOfMemoryOutcome.java#L65)
triggers an OutOfMemoryException during ["next"
allocation](https://github.com/apache/drill/blob/master/exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/ScanBatch.java#L172),
and all tests are expected to fail.
And then, there is the [third
case](https://github.com/apache/drill/blob/master/exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/ScanBatch.java#L247),
which is a general catch (e.g.`reader.next()` throws OutOfMemoryException).
And as you mentioned, readers cannot unwind, so that correctly fails the
fragment.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [email protected] or file a JIRA ticket
with INFRA.
---
