----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/52559/#review152659 -----------------------------------------------------------
The 8th version looks good to me. +1 - Yongzhi Chen On Oct. 13, 2016, 11:38 p.m., Chaoyu Tang wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/52559/ > ----------------------------------------------------------- > > (Updated Oct. 13, 2016, 11:38 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 > >
