[
https://issues.apache.org/jira/browse/DRILL-5083?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15704314#comment-15704314
]
Paul Rogers commented on DRILL-5083:
------------------------------------
It seems that only the {{RecordIterator}} has the {{clearInflightBatches( )}}
behavior.
Also seems that the ScreenCreator.ScreenRoot iterates though all operators for
the fragment and closes them one by one, from the top down.
But, when we get to the MergeJoin, it is the merge join's close( ) itself that
close the left and right inputs (which, in my test case, were record
iterators.) Is this necessary? Won't the ScreenRoot do the closing? Stepping
through the code shows that it will, so we're closing the same operator
multiple times.
The record iterator appears to buffer batches to allow moderate rewinding. If
so, then does it really need to call {{next()}} on its incoming operator? Why
not let the incoming operator clean up it's own mess in close? Code in question:
{code}
while (lastOutcome == IterOutcome.OK || lastOutcome ==
IterOutcome.OK_NEW_SCHEMA) {
// Clear all buffers from incoming.
for (VectorWrapper<?> wrapper : incoming) {
wrapper.getValueVector().clear();
}
lastOutcome = incoming.next();
}
{code}
Note that this is a loop to read from incoming while data is ready. But, if we
are on a branch that didn't fail, the incoming does not know that an error
condition occurred and will operate as normal. (Or so it seems.)
Tracing through the code, cleanup seems to stay more-or-less on track through
upper (failed) branch of the first merge join. (The iterator validator throws
an exception when the record iterator tries to call next.)
But, in the lower branch, we have another merge that doesn't know that the
query failed. It is in its start state. As it turns out, this merge is stopped
via the STOP code shown above.
Yet another interesting fact is that, somehow, the failed operators got put on
the end of the list of operators to close. That is, the external sort, which
ran out of memory and is holding onto in-memory batches, was closed last,
releasing its (now useless) memory as the last operation. Seems this should be
done first (or at least in proper top-down order.)
Perhaps the close methods should be extended to flag redundant closes to catch
such problems.
> IteratorValidator does not handle RecordIterator cleanup call to next( )
> ------------------------------------------------------------------------
>
> Key: DRILL-5083
> URL: https://issues.apache.org/jira/browse/DRILL-5083
> Project: Apache Drill
> Issue Type: Bug
> Affects Versions: 1.8.0
> Reporter: Paul Rogers
> Priority: Minor
>
> This one is very confusing...
> In a test with a MergeJoin and external sort, operators are stacked something
> like this:
> {code}
> Screen
> - MergeJoin
> - - External Sort
> ...
> {code}
> Using the injector to force a OOM in spill, the external sort threw a
> UserException up the stack. This was handed by:
> {code}
> IteratorValidatorBatchIterator.next( )
> RecordIterator.clearInflightBatches( )
> RecordIterator.close( )
> MergeJoinBatch.close( )
> {code}
> Which does the following:
> {code}
> // Check whether next() should even have been called in current state.
> if (null != exceptionState) {
> throw new IllegalStateException(
> {code}
> But, the exceptionState is set, so we end up throwing an
> IllegalStateException during cleanup.
> Seems the code should agree: if {{next( )}} will be called during cleanup,
> then {{next( )}} should gracefully handle that case.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
