----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/61010/#review181196 -----------------------------------------------------------
Just 2 comments and they are not that important. Feel free to tell me if you disagree. Even without change I think the code is ready to commit. Thanks for the patch, and the changes! Peter itests/hive-unit/src/test/java/org/apache/hive/service/cli/operation/TestOperationLoggingLayout.java Line 123 (original), 124 (patched) <https://reviews.apache.org/r/61010/#comment256683> I might be too pedantic, so feel free to tell me if you do not agree with me here :) Honestly! It might be good to check here the closure of the LogDivertAppenderForTest too, so if something changes later we can be sure, that all of the appenders are closed. If might be good to rename the test class to TestOpertionLogging, since from now on this will not only test the layout, but the appender closure too. - Peter Vary On July 22, 2017, 12:16 a.m., Andrew Sherman wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/61010/ > ----------------------------------------------------------- > > (Updated July 22, 2017, 12:16 a.m.) > > > Review request for hive. > > > Repository: hive-git > > > Description > ------- > > Previously HIVE-16061 and HIVE-16400 changed Operation Logging to use the > Log4j2 > RoutingAppender to automatically output the log for each query into an > individual operation log file. As log4j does not know when a query is > finished > it keeps the OutputStream in the subordinate Appender open even when the query > completes. The stream holds a file descriptor and so we leak file > descriptors. > Note that we are already careful to close any streams reading from the > operation > log file. To fix this we use a technique described in the comments of > LOG4J2-510 which uses reflection to close the subordinate Appender. We use > this > to close the per-query subordinate Appenders from both LogDivertAppender and > LogDivertAppenderForTest. The test in TestOperationLoggingLayout is extended > to > check that the Appenders are closed when a query completes. > > > Diffs > ----- > > common/src/java/org/apache/hadoop/hive/common/LogUtils.java > 83f3af7440253bfbcedbc8b21d745fb71c0d7fb9 > > itests/hive-unit/src/test/java/org/apache/hive/service/cli/operation/TestOperationLoggingLayout.java > 1a8337f574bb753e8c3c48a6b477b17700b05256 > ql/src/java/org/apache/hadoop/hive/ql/log/LogDivertAppender.java > e697b545984555414e27bafe92d7f22829a22687 > ql/src/java/org/apache/hadoop/hive/ql/log/LogDivertAppenderForTest.java > 465844d66b92b371f457fda0d885d75fbfce6805 > service/src/java/org/apache/hive/service/cli/operation/Operation.java > 8d453d5d9153c2ec86c4adc7a68bd3b5dd249743 > > > Diff: https://reviews.apache.org/r/61010/diff/2/ > > > Testing > ------- > > Hand testing to show leak has gone. > The test in TestOperationLoggingLayout is extended to check that the Appender > is closed. > > > Thanks, > > Andrew Sherman > >
