hive-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Ashutosh Chauhan" <hashut...@apache.org>
Subject Re: Review Request: HIVE-4513 - disable hivehistory logs by default
Date Mon, 03 Jun 2013 21:03:15 GMT

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



data/conf/hive-site.xml
<https://reviews.apache.org/r/11029/#comment44263>

    Is there a reason for this to be set to true for tests? Unless there is, we should set
config in tests to the default values, since we should test default configs.



ql/src/java/org/apache/hadoop/hive/ql/history/HiveHistory.java
<https://reviews.apache.org/r/11029/#comment44264>

    doesn't read right. I guess you wanted 
    ... statistics into a file.



ql/src/java/org/apache/hadoop/hive/ql/history/HiveHistory.java
<https://reviews.apache.org/r/11029/#comment44266>

    This is existing comment which doesnt read right. But since we are doing major surgery
on HiveHistory, it will be good to update to make it more sensible.



ql/src/java/org/apache/hadoop/hive/ql/history/HiveHistory.java
<https://reviews.apache.org/r/11029/#comment44268>

    I think word job is not required in this comment.



ql/src/java/org/apache/hadoop/hive/ql/history/HiveHistory.java
<https://reviews.apache.org/r/11029/#comment44269>

    I think query is a better word than job here.



ql/src/java/org/apache/hadoop/hive/ql/history/HiveHistory.java
<https://reviews.apache.org/r/11029/#comment44270>

    Better worded as
    Called at the end of query.



ql/src/java/org/apache/hadoop/hive/ql/history/HiveHistory.java
<https://reviews.apache.org/r/11029/#comment44271>

    Again use of word job is confusing, we shall use query here as well.



ql/src/java/org/apache/hadoop/hive/ql/history/HiveHistory.java
<https://reviews.apache.org/r/11029/#comment44272>

    Incorrect comment.



ql/src/java/org/apache/hadoop/hive/ql/history/HiveHistory.java
<https://reviews.apache.org/r/11029/#comment44274>

    Function name is IdtoTable, but comment says table to id. One of this needs to be corrected.



ql/src/java/org/apache/hadoop/hive/ql/history/HiveHistoryImpl.java
<https://reviews.apache.org/r/11029/#comment44275>

    Similar comment as in HiveHistory.java



ql/src/java/org/apache/hadoop/hive/ql/history/HiveHistoryImpl.java
<https://reviews.apache.org/r/11029/#comment44277>

    Should this be hive.ql.exec.HiveHistoryImpl to avoid confusion?



ql/src/java/org/apache/hadoop/hive/ql/history/HiveHistoryImpl.java
<https://reviews.apache.org/r/11029/#comment44278>

    and instead of an ?



ql/src/java/org/apache/hadoop/hive/ql/history/HiveHistoryImpl.java
<https://reviews.apache.org/r/11029/#comment44280>

    In case of incorrect config, should this throw an exception instead of silent return,
otherwise there will be errors later when something is tried to be written in history file.



ql/src/java/org/apache/hadoop/hive/ql/history/HiveHistoryImpl.java
<https://reviews.apache.org/r/11029/#comment44281>

    Same comment as above.



ql/src/java/org/apache/hadoop/hive/ql/history/HiveHistoryImpl.java
<https://reviews.apache.org/r/11029/#comment44283>

    This should be static class variable, otherwise nextInt() will return same value for each
invocation.



ql/src/java/org/apache/hadoop/hive/ql/history/HiveHistoryImpl.java
<https://reviews.apache.org/r/11029/#comment44284>

    Instead of / we shall use File.Seprator



ql/src/java/org/apache/hadoop/hive/ql/history/HiveHistoryImpl.java
<https://reviews.apache.org/r/11029/#comment44287>

    Consider using File.createNewFile here.



ql/src/java/org/apache/hadoop/hive/ql/history/HiveHistoryImpl.java
<https://reviews.apache.org/r/11029/#comment44288>

    Use  System.getProperty("line.separator") instead of \n



ql/src/java/org/apache/hadoop/hive/ql/history/HiveHistoryImpl.java
<https://reviews.apache.org/r/11029/#comment44289>

    start of query ?



ql/src/java/org/apache/hadoop/hive/ql/history/HiveHistoryUtil.java
<https://reviews.apache.org/r/11029/#comment44291>

    Missing apache header



ql/src/java/org/apache/hadoop/hive/ql/history/HiveHistoryViewer.java
<https://reviews.apache.org/r/11029/#comment44292>

    HiveHistoryViewer.class


- Ashutosh Chauhan


On May 13, 2013, 10:12 p.m., Thejas Nair wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/11029/
> -----------------------------------------------------------
> 
> (Updated May 13, 2013, 10:12 p.m.)
> 
> 
> Review request for hive.
> 
> 
> Description
> -------
> 
> HiveHistory log files (hive_job_log_hive_*.txt files) store information about hive query
such as query string, plan , counters and MR job progress information.
> 
> There is no mechanism to delete these files and as a result they get accumulated over
time, using up lot of disk space. 
> I don't think this is used by most people, so I think it would better to turn this off
by default. Jobtracker logs already capture most of this information, though it is not as
structured as history logs.
> 
> The change :
> A new config parameter hive.session.history.enabled controls if the history-log is enabled.
By default it is set to false.
> SessionState initializes the HiveHIstory object. When this config is set to false, it
creates a Proxy object that does not do anything. I did this instead of having SessionState
return null, because that would add null checks in too many places. This keeps the code cleaner
and avoids possibility of NPE.
> As the proxy only works against interfaces, i created a HiveHistory interface, moved
the implementation to HiveHistoryImpl. static functions were moved to HiveHistoryUtil .
> 
> 
> This addresses bug HIVE-4513.
>     https://issues.apache.org/jira/browse/HIVE-4513
> 
> 
> Diffs
> -----
> 
>   common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 1672453 
>   conf/hive-default.xml.template 3a7d1dc 
>   data/conf/hive-site.xml 544ba35 
>   ql/src/java/org/apache/hadoop/hive/ql/history/HiveHistory.java e1c1ae3 
>   ql/src/java/org/apache/hadoop/hive/ql/history/HiveHistoryImpl.java PRE-CREATION 
>   ql/src/java/org/apache/hadoop/hive/ql/history/HiveHistoryProxyHandler.java PRE-CREATION

>   ql/src/java/org/apache/hadoop/hive/ql/history/HiveHistoryUtil.java PRE-CREATION 
>   ql/src/java/org/apache/hadoop/hive/ql/history/HiveHistoryViewer.java fdd56db 
>   ql/src/java/org/apache/hadoop/hive/ql/session/SessionState.java 3d43451 
>   ql/src/test/org/apache/hadoop/hive/ql/history/TestHiveHistory.java a783303 
> 
> Diff: https://reviews.apache.org/r/11029/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Thejas Nair
> 
>


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