> On July 20, 2017, 11:59 p.m., Aihua Xu wrote: > > service/src/java/org/apache/hive/service/cli/operation/Operation.java > > Lines 269 (patched) > > <https://reviews.apache.org/r/61010/diff/1/?file=1780484#file1780484line269> > > > > We only register the Operation log appender once for all the operation > > logs at the beginning. Now if you stop the appender here, then the > > following queries will not be able to output to the appender any more, > > right? > > > > Can you test your patch by: connect to HS2 from one beeline session, > > disconnect and reconnect. Then see if you still see output from the beeline > > console? > > > > Will we be able to close OutputStream instead? stopQueryAppender() > > should be called when HS2 service gets shutdown. > > Peter Vary wrote: > Nice catch Andrew! > > One more thing. Could you please do this for the LogDivertAppenderForTest > too? It is only used for tests, but it would be good to clean up it too. > > Thanks, > Peter > > Andrew Sherman wrote: > @Aihua thanks for the stimulating question. I ran hand tests to prove > that logging works for multiple queries in the same session, and also in a > new session. The reason the code is OK is that it is not the RoutineAppender > that is closed, but the specific Appender for the query. In > https://logging.apache.org/log4j/2.x/manual/appenders.html#RoutingAppender > this Appender is referred to as a subordinate Appender. I'm updating the code > to make this clearer. > > @Peter I will look at LogDivertAppenderForTest to see if I can do the > same thing there.
I see. That makes sense. I will take a second look. Thanks for the explanation. - Aihua ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/61010/#review181083 ----------------------------------------------------------- On July 20, 2017, 10:45 p.m., Andrew Sherman wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/61010/ > ----------------------------------------------------------- > > (Updated July 20, 2017, 10:45 p.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 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 appender. The test in TestOperationLoggingLayout > is > extended to check that the Appender is closed. > > > Diffs > ----- > > > 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 > service/src/java/org/apache/hive/service/cli/operation/Operation.java > 8d453d5d9153c2ec86c4adc7a68bd3b5dd249743 > > > Diff: https://reviews.apache.org/r/61010/diff/1/ > > > Testing > ------- > > Hand testing to show leak has gone. > The test in TestOperationLoggingLayout is extended to check that the Appender > is closed. > > > Thanks, > > Andrew Sherman > >
