----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/52559/#review151537 -----------------------------------------------------------
common/src/java/org/apache/hadoop/hive/conf/HiveConf.java (line 3144) <https://reviews.apache.org/r/52559/#comment219978> nit: needs a better explanation from a user's perspective... user doesn't know what is a driver and why would it be closed is not clear here ql/src/java/org/apache/hadoop/hive/ql/Driver.java (line 344) <https://reviews.apache.org/r/52559/#comment219979> is this called from elsewhere too? the method existed before ql/src/java/org/apache/hadoop/hive/ql/Driver.java (line 556) <https://reviews.apache.org/r/52559/#comment219980> iirc if you catch InterruptException, in the catch block the interrupt flag would be reset, so this will not return true ql/src/java/org/apache/hadoop/hive/ql/Driver.java (line 1145) <https://reviews.apache.org/r/52559/#comment219981> is it possible to add a lock object for this and add javadoc to make it explicit what it covers? I doesn't look like all ctx accesses are covered. ql/src/java/org/apache/hadoop/hive/ql/Driver.java (line 1392) <https://reviews.apache.org/r/52559/#comment219982> nit: whitespace here and below ql/src/java/org/apache/hadoop/hive/ql/Driver.java (line 1902) <https://reviews.apache.org/r/52559/#comment219983> same thing wrt interrupt state ql/src/java/org/apache/hadoop/hive/ql/Driver.java (line 2221) <https://reviews.apache.org/r/52559/#comment219984> nit: might as well just pass TimeUnit.MILLISECONDS instead of * 1000 ql/src/java/org/apache/hadoop/hive/ql/Driver.java (line 2227) <https://reviews.apache.org/r/52559/#comment219990> 1) these states do not appear thread-safe themselves 2) why are so many states needed, by the way? e.g. what difference does it make for termination if it's compiling or running? - Sergey Shelukhin On Oct. 5, 2016, 4:56 p.m., Chaoyu Tang wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/52559/ > ----------------------------------------------------------- > > (Updated Oct. 5, 2016, 4:56 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 > ----- > > common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 4c3ef3e > 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 > >
