> On July 24, 2017, 9:09 a.m., Peter Vary wrote: > > 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/diff/1-2/?file=1780482#file1780482line124> > > > > 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. > > Andrew Sherman wrote: > Thanks for the comments which I do not find pedantic. > But in the case of the test I think I am testing the closure of the > LogDivertAppenderForTest too (see lines 124-128). Can you take another look > when you have time? > Thanks > -Andrew
That's right, I do not know how I missed those lines :) - Peter ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/61010/#review181196 ----------------------------------------------------------- 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 > >
