> On Oct. 12, 2016, 1:25 a.m., Sergey Shelukhin wrote: > > ql/src/java/org/apache/hadoop/hive/ql/Driver.java, line 617 > > <https://reviews.apache.org/r/52559/diff/6/?file=1531289#file1531289line617> > > > > this will overwrite the previous error (this being finally) without > > logging it (other than to console). In fact the original error if any is > > probably more useful to the user...
Yeah, good catch, I converted them to LOG.warn and write them to the log. > On Oct. 12, 2016, 1:25 a.m., Sergey Shelukhin wrote: > > ql/src/java/org/apache/hadoop/hive/ql/Driver.java, line 1424 > > <https://reviews.apache.org/r/52559/diff/6/?file=1531289#file1531289line1424> > > > > nit: typo > > > > also is it really true for every other possible state? change the error msg "FAILED: Precompiled query has been cancelled or closed". Yes, the query should has been successfully compiled when user call this API since passes the flag alreadyCompiled value true > On Oct. 12, 2016, 1:25 a.m., Sergey Shelukhin wrote: > > ql/src/java/org/apache/hadoop/hive/ql/Driver.java, line 635 > > <https://reviews.apache.org/r/52559/diff/6/?file=1531289#file1531289line635> > > > > this ignores downstreamError. By design? Same for similar code below. > > The existing code with this pattern returns immediately in the catch that > > sets downstreamError, but here the execution can continue from the above > > catch The existing code also has the finally block to invoke hooks, do perfLog etc after the catch which sets the downstreamError, this patch is only to add deferred close if applicable and do the state transition atomically. > On Oct. 12, 2016, 1:25 a.m., Sergey Shelukhin wrote: > > ql/src/java/org/apache/hadoop/hive/ql/Driver.java, line 1582 > > <https://reviews.apache.org/r/52559/diff/6/?file=1531289#file1531289line1582> > > > > some interrupt-related return paths in the try above will result in > > isFinishedWithError being true here... it seems like that's by design right > > now because error and interrupt conditions are not distinguished here. It > > might be helpful to adcd a comment added the comment to variable isFinishedWithError > On Oct. 12, 2016, 1:25 a.m., Sergey Shelukhin wrote: > > ql/src/java/org/apache/hadoop/hive/ql/Driver.java, line 2015 > > <https://reviews.apache.org/r/52559/diff/6/?file=1531289#file1531289line2015> > > > > same as above - should we return here? seems like the below ignores > > downstreamError on some paths see comments above > On Oct. 12, 2016, 1:25 a.m., Sergey Shelukhin wrote: > > ql/src/java/org/apache/hadoop/hive/ql/Driver.java, line 2271 > > <https://reviews.apache.org/r/52559/diff/6/?file=1531289#file1531289line2271> > > > > does it have to throw here or can it just exit? the old code seems to > > only do stuff when in the right state, and nothing in the wrong state I think it might be more helpful to raise the exception here if this API is called in a wrong state. For example, the existing getResult(..) checkes the state: if (destroyed) { throw new IOException("FAILED: Operation cancelled"); } > On Oct. 12, 2016, 1:25 a.m., Sergey Shelukhin wrote: > > ql/src/java/org/apache/hadoop/hive/ql/Driver.java, line 2418 > > <https://reviews.apache.org/r/52559/diff/6/?file=1531289#file1531289line2418> > > > > I don't see an explicit check for this state anywhere in the code, > > except for some error paths. What is setting driver state to destroyed > > supposed to trigger? In getResult(..) or resetFetch(), this state is explicitly checked. If the destroy is called, the driver will do the final cleaning up and after that, driver could no more be useful. - Chaoyu ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/52559/#review152243 ----------------------------------------------------------- On Oct. 11, 2016, 10:12 p.m., Chaoyu Tang wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/52559/ > ----------------------------------------------------------- > > (Updated Oct. 11, 2016, 10:12 p.m.) > > > Review request for hive, Sergey Shelukhin, Thejas Nair, Vaibhav Gumashta, and > Yongzhi Chen. > > > Bugs: HIVE-14799 > https://issues.apache.org/jira/browse/HIVE-14799 > > > Repository: hive-git > > > Description > ------- > > This patch is going to fix a couple of Driver issues related to the close > request from a thread other than the one running the query (e.g. from > SQLOperation cancel via Timeout or Ctrl-C): > 1. Driver is not thread safe and usually supports only one thread at time > since it has variables like ctx, plan which are not thread protected. But > certain special use cases need access the Driver objects from multiply > threads. For example, when a query runs in a background thread, driver.close > is invoked in another thread by the query timeout (see HIVE-4924). The close > process could nullify the shared variables like ctx which could cause NPE in > the other query thread which is using them. This runtime exception is > unpredictable and not well handled in the code. Some resources (e.g. locks, > files) are left behind and not be cleaned because there are no more available > = references to them. In this patch, I use the waiting in the close which > makes sure only one thread uses these variables and the resource cleaning > happens after the query finished (or interrupted). > 2. SQLOperation.cancel sends the interrupt signal to the background thread > running the query (via backgroundHandle.cancel(true)) but it could not stop > that process since there is no code to capture the signal in the process. In > another word, current timeout code could not gracefully and promptly stop the > query process, though it could eventually stop the process by killing the > running tasks (e.g. MapRedTask) via driverContext.shutdown (see HIVE-5901). > So in the patch, I added a couple of checkpoints to intercept the interrupt > signal either set by close method (a volatile variable) or thread.interrupt. > They should be helpful to capture these signals earlier , though not > intermediately. > > > Diffs > ----- > > ql/src/java/org/apache/hadoop/hive/ql/Driver.java dd55434 > > Diff: https://reviews.apache.org/r/52559/diff/ > > > Testing > ------- > > Manually tests > Precommit tests > > > Thanks, > > Chaoyu Tang > >
