> On Oct. 17, 2017, 11:25 p.m., Aihua Xu wrote: > > standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/ObjectStore.java > > Lines 261 (patched) > > <https://reviews.apache.org/r/63043/diff/1/?file=1857700#file1857700line262> > > > > OK. I started to recall why we did that this way. I think we don't want > > to call query.close() in the subclass since when you return query.execute() > > actually DN doesn't get the result yet until you consume those results. > > > > That's why we want the caller to be responsible to call close() > > explictly and the caller knows that it has consumed all the data. > > > > The new implmentation will close the query object inside the subclass > > right? > > Alexander Kolbasov wrote: > Originally Query wasn't Closeable or AutoCLoseable, so you couldn't use > it anyway. Now it is and it could be possible to use Query directly, but this > raises a problem of handling exceptions on close. > > Answering the second part of your question. The query now is closed > immediately after the use (in subclass that creates the query). Query results > can be used after query close, but I need to copy them into another > collection - that's why there are a bunch of changes like `return new > ArrayList<>(result)`. > > Aihua Xu wrote: > I see. I believe the original solution is to avoid the memory pressure on > HMS. With your approach, we are prefetching all the data and save in a > collection, while the original approach delays the data retrieval until it's > needed. > > Is that possible to preserve that logic but still let it close > automatically?
Note that we are not copying the data itself in the new ArrayList, we are copying pointers. So the only extra overhead is the newly allocated array which is mostly a short-living object and not a big deal. The prefetches are present in the old code - I didn't add any new ones. Doing what you suggest requires bigger code refactoring since try-with-resource is based on try-blocks, so we need to significantly shuffle the code around. - Alexander ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/63043/#review188419 ----------------------------------------------------------- On Oct. 16, 2017, 6:37 p.m., Alexander Kolbasov wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/63043/ > ----------------------------------------------------------- > > (Updated Oct. 16, 2017, 6:37 p.m.) > > > Review request for hive, Aihua Xu, Andrew Sherman, Alan Gates, Janaki > Lahorani, Sergio Pena, Sahil Takiar, and Vihang Karajgaonkar. > > > Bugs: 17730 > https://issues.apache.org/jira/browse/17730 > > > Repository: hive-git > > > Description > ------- > > HIVE-17730 Queries can be closed automatically > > The goal of the patch is to replaces all curret uses of QueryWrapper with > try-with-resource construct that guarantees that the > query is closed. > > This change affects the scope of the open query, so in all cases where the > code was returning the list of query results, I had to copy the list to a new > ArrayList to allow access after query close. > > > Diffs > ----- > > metastore/src/java/org/apache/hadoop/hive/metastore/tools/HiveMetaTool.java > 22e246f1c914532ed6c67151c261fa709a283ef2 > > standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/ObjectStore.java > ffb2abdf6294dbd470525ad888bd95feac7e7f06 > > > Diff: https://reviews.apache.org/r/63043/diff/1/ > > > Testing > ------- > > > Thanks, > > Alexander Kolbasov > >
