hive-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Andrew Sherman <asher...@cloudera.com>
Subject Re: Review Request 61010: HIVE-17128 Operation Logging leaks file descriptors as the log4j Appender is never closed
Date Fri, 21 Jul 2017 18:41:54 GMT


> 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

@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.


- Andrew


-----------------------------------------------------------
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
> 
>


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