hive-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Sergey Shelukhin <ser...@hortonworks.com>
Subject Re: Review Request 48886: HIVE-14052: Cleanup of structures required when LLAP access from external clients completes
Date Thu, 30 Jun 2016 23:22:02 GMT

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/48886/#review140263
-----------------------------------------------------------




llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/QueryTracker.java (line 103)
<https://reviews.apache.org/r/48886/#comment205615>

    is this for all fragments, or only external ones? Should say and handle appropriately
in get if it's only for external.



llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/QueryTracker.java (line 158)
<https://reviews.apache.org/r/48886/#comment205619>

    hmm... was this patch updated? this can throw in many circumstances.
    Why do we have to do it like this anyway? It seems that the query stuff will repeatedly
be cleaned up and recreated.



llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/QueryTracker.java (line 217)
<https://reviews.apache.org/r/48886/#comment205617>

    nit



llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/QueryTracker.java (line 232)
<https://reviews.apache.org/r/48886/#comment205620>

    can fragments start before the cleanip task runs and cause it to be invalid? I think it
might make more sense to have delay before even considering the cleanup...



llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/QueryTracker.java (line 248)
<https://reviews.apache.org/r/48886/#comment205621>

    nit: constant?



llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/QueryTracker.java (line 293)
<https://reviews.apache.org/r/48886/#comment205623>

    I don't think wait on Future is a real method, it's just Object::wait; get() should be
called.
    
    Also timeout might be helpful



llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/QueryTracker.java (line 416)
<https://reviews.apache.org/r/48886/#comment205624>

    nit: unneeded?



llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/QueryTracker.java (line 421)
<https://reviews.apache.org/r/48886/#comment205625>

    should this be checked during the attempt to get the child locks?


- Sergey Shelukhin


On June 17, 2016, 10:31 p.m., Jason Dere wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/48886/
> -----------------------------------------------------------
> 
> (Updated June 17, 2016, 10:31 p.m.)
> 
> 
> Review request for hive and Siddharth Seth.
> 
> 
> Bugs: HIVE-14052
>     https://issues.apache.org/jira/browse/HIVE-14052
> 
> 
> Repository: hive-git
> 
> 
> Description
> -------
> 
> Add a hook to call run QueryTracker.queryComplete if there are no more fragments for
this query.
> This cleanup runs on delay and can be cancelled if another fragment request comes in
with the same query ID.
> 
> 
> Diffs
> -----
> 
>   llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/ContainerRunnerImpl.java
ded84c1 
>   llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/LlapDaemon.java c7e9d32

>   llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/QueryTracker.java a965872

>   ql/src/java/org/apache/hadoop/hive/llap/LlapOutputFormatService.java 825488f 
>   ql/src/test/org/apache/hadoop/hive/llap/TestLlapOutputFormat.java 2288cd4 
> 
> Diff: https://reviews.apache.org/r/48886/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Jason Dere
> 
>


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