hive-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Alexander Kolbasov <ako...@gmail.com>
Subject Re: Review Request 63043: HIVE-17730 Queries can be closed automatically
Date Wed, 18 Oct 2017 17:24:03 GMT


> 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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message