Re: Review Request: HIVE-4513 - disable hivehistory logs by default
--- 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.
Re: Review Request: HIVE-4513 - disable hivehistory logs by default
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/11029/ --- (Updated May 13, 2013, 8:13 p.m.) Review request for hive. Summary (updated) - HIVE-4513 - disable hivehistory logs by default Description --- HIVE-4513 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
Re: Review Request: HIVE-4513 - disable hivehistory logs by default
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/11029/ --- (Updated May 13, 2013, 9:51 p.m.) Review request for hive. Changes --- Changes in new patch - add @Override to interface functions being implemented in HiveHistoryImpl Removing javadoc duplication in HiveHistoryImpl. It will automatically inherit the documentation from interface. Logging the exception in code unrelated to patch, to partly address Brock's concern. Since the code is not part of the patch, I don't want to increase the scope to address that concern. Description --- HIVE-4513 This addresses bug HIVE-4513. https://issues.apache.org/jira/browse/HIVE-4513 Diffs (updated) - 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
Re: Review Request: HIVE-4513 - disable hivehistory logs by default
--- 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. Changes --- Updating review with background of the changes. Description (updated) --- 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
Re: Review Request: HIVE-4513 - disable hivehistory logs by default
On May 9, 2013, 4:37 p.m., Brock Noland wrote: ql/src/java/org/apache/hadoop/hive/ql/history/HiveHistoryViewer.java, lines 71-73 https://reviews.apache.org/r/11029/diff/1/?file=289274#file289274line71 This is bad... I know it's not related to your change but can we fix this? I have made things slightly better by logging the error. I looked at throwing an exception, but that would need changes in other classes to handle the exception correctly (Such as hive web interface classes). Since this code is unrelated to the patch, and it is not a 1-2 liner, I think we should address that separately. - Thejas --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/11029/#review20380 --- 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
