Re: Review Request 65075: HIVE-18426: Memory leak in RoutingAppender for every hive operation

2018-01-24 Thread kalyan kumar kalvagadda via Review Board 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65075/
---

(Updated Jan. 24, 2018, 7:28 p.m.)


Review request for hive, Aihua Xu, Andrew Sherman, and Sergio Pena.


Changes
---

Addressed some more codestyle errors.


Bugs: HIVE-18426
https://issues.apache.org/jira/browse/HIVE-18426


Repository: hive-git


Description
---

Each new operation creates new entry in the ConcurrentMap in RoutingAppender 
but when the operation ends, AppenderControl stored in the map is retrieved and 
stopped but the entry in ConcurrentMap is never cleaned up.


Diffs (updated)
-

  common/src/java/org/apache/hadoop/hive/common/LogUtils.java 
5c7ec69f04587ab18704a10fade6e3a47601a87f 
  
itests/hive-unit/src/test/java/org/apache/hive/service/cli/operation/TestOperationLoggingLayout.java
 8febe3e79ff892c54b696b6c6ef92f7026c46033 


Diff: https://reviews.apache.org/r/65075/diff/5/

Changes: https://reviews.apache.org/r/65075/diff/4-5/


Testing
---

Made sure that the new tests updated to verify this change are passing.


Thanks,

kalyan kumar kalvagadda

Re: Review Request 65075: HIVE-18426: Memory leak in RoutingAppender for every hive operation

2018-01-24 Thread kalyan kumar kalvagadda via Review Board 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65075/
---

(Updated Jan. 24, 2018, 5:44 p.m.)


Review request for hive, Aihua Xu, Andrew Sherman, and Sergio Pena.


Changes
---

Added code changes to addess the codestyle errors


Bugs: HIVE-18426
https://issues.apache.org/jira/browse/HIVE-18426


Repository: hive-git


Description
---

Each new operation creates new entry in the ConcurrentMap in RoutingAppender 
but when the operation ends, AppenderControl stored in the map is retrieved and 
stopped but the entry in ConcurrentMap is never cleaned up.


Diffs (updated)
-

  common/src/java/org/apache/hadoop/hive/common/LogUtils.java 
5c7ec69f04587ab18704a10fade6e3a47601a87f 
  
itests/hive-unit/src/test/java/org/apache/hive/service/cli/operation/TestOperationLoggingLayout.java
 8febe3e79ff892c54b696b6c6ef92f7026c46033 


Diff: https://reviews.apache.org/r/65075/diff/4/

Changes: https://reviews.apache.org/r/65075/diff/3-4/


Testing
---

Made sure that the new tests updated to verify this change are passing.


Thanks,

kalyan kumar kalvagadda

Re: Review Request 65075: HIVE-18426: Memory leak in RoutingAppender for every hive operation

2018-01-23 Thread kalyan kumar kalvagadda via Review Board 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65075/
---

(Updated Jan. 23, 2018, 8:27 p.m.)


Review request for hive, Aihua Xu, Andrew Sherman, and Sergio Pena.


Changes
---

Addressed comments by adding new test to verify 
HushableRandomAccessFileAppender.


Bugs: HIVE-18426
https://issues.apache.org/jira/browse/HIVE-18426


Repository: hive-git


Description
---

Each new operation creates new entry in the ConcurrentMap in RoutingAppender 
but when the operation ends, AppenderControl stored in the map is retrieved and 
stopped but the entry in ConcurrentMap is never cleaned up.


Diffs (updated)
-

  common/src/java/org/apache/hadoop/hive/common/LogUtils.java 
5c7ec69f04587ab18704a10fade6e3a47601a87f 
  
itests/hive-unit/src/test/java/org/apache/hive/service/cli/operation/TestOperationLoggingLayout.java
 8febe3e79ff892c54b696b6c6ef92f7026c46033 


Diff: https://reviews.apache.org/r/65075/diff/3/

Changes: https://reviews.apache.org/r/65075/diff/2-3/


Testing
---

Made sure that the new tests updated to verify this change are passing.


Thanks,

kalyan kumar kalvagadda

Re: Review Request 65075: HIVE-18426: Memory leak in RoutingAppender for every hive operation

2018-01-23 Thread kalyan kumar kalvagadda via Review Board 


> On Jan. 11, 2018, 9:51 p.m., Andrew Sherman wrote:
> > itests/hive-unit/src/test/java/org/apache/hive/service/cli/operation/TestOperationLoggingLayout.java
> > Line 162 (original), 163 (patched)
> > 
> >
> > This code was to check the case described in 
> > https://issues.apache.org/jira/browse/HIVE-17826?focusedCommentId=16208636=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-16208636
> > I think this is no longer tested. Is that OK?
> 
> kalyan kumar kalvagadda wrote:
> I have couple of observations with the limited knowledge that I have. 
> Correct me if i'm wrong here.
> 
> 1. HIVE-17128: Code changes done as part of this jira makes sure that 
> log4j Appender is closed when operation is closed to avoid file descriptors.
> 2. HIVE-17826: Added HushableRandomAccessFileAppender which is a copy of 
> RandomAccessFileAppender but has a explicit check in append() method to see 
> if the appender is closed.
> 
>  I think issue reported in HIVE-17826 is seen only because the 
> RoutingAppender is not properly cleaned-up when the operation is stopped. If 
> we have the patch i submitted we may not see the issue reported in HIVE-17826 
> even with out the fix of adding HushableRandomAccessFileAppender.
> 
> Andrew Sherman wrote:
> OK

I have added new test to verify HushableRandomAccessFileAppender


- kalyan kumar


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65075/#review195255
---


On Jan. 12, 2018, 8:09 p.m., kalyan kumar kalvagadda wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65075/
> ---
> 
> (Updated Jan. 12, 2018, 8:09 p.m.)
> 
> 
> Review request for hive, Aihua Xu, Andrew Sherman, and Sergio Pena.
> 
> 
> Bugs: HIVE-18426
> https://issues.apache.org/jira/browse/HIVE-18426
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> Each new operation creates new entry in the ConcurrentMap in RoutingAppender 
> but when the operation ends, AppenderControl stored in the map is retrieved 
> and stopped but the entry in ConcurrentMap is never cleaned up.
> 
> 
> Diffs
> -
> 
>   common/src/java/org/apache/hadoop/hive/common/LogUtils.java 
> 0a3e0c72011951b6b1543352308bd51233c847fb 
>   
> itests/hive-unit/src/test/java/org/apache/hive/service/cli/operation/TestOperationLoggingLayout.java
>  8febe3e79ff892c54b696b6c6ef92f7026c46033 
> 
> 
> Diff: https://reviews.apache.org/r/65075/diff/2/
> 
> 
> Testing
> ---
> 
> Made sure that the new tests updated to verify this change are passing.
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

Re: Review Request 65075: HIVE-18426: Memory leak in RoutingAppender for every hive operation

2018-01-12 Thread kalyan kumar kalvagadda via Review Board 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65075/
---

(Updated Jan. 12, 2018, 8:09 p.m.)


Review request for hive, Aihua Xu, Andrew Sherman, and Sergio Pena.


Changes
---

Addressed comments.


Bugs: HIVE-18426
https://issues.apache.org/jira/browse/HIVE-18426


Repository: hive-git


Description
---

Each new operation creates new entry in the ConcurrentMap in RoutingAppender 
but when the operation ends, AppenderControl stored in the map is retrieved and 
stopped but the entry in ConcurrentMap is never cleaned up.


Diffs (updated)
-

  common/src/java/org/apache/hadoop/hive/common/LogUtils.java 
0a3e0c72011951b6b1543352308bd51233c847fb 
  
itests/hive-unit/src/test/java/org/apache/hive/service/cli/operation/TestOperationLoggingLayout.java
 8febe3e79ff892c54b696b6c6ef92f7026c46033 


Diff: https://reviews.apache.org/r/65075/diff/2/

Changes: https://reviews.apache.org/r/65075/diff/1-2/


Testing
---

Made sure that the new tests updated to verify this change are passing.


Thanks,

kalyan kumar kalvagadda

Re: Review Request 65075: HIVE-18426: Memory leak in RoutingAppender for every hive operation

2018-01-12 Thread Andrew Sherman via Review Board 


> On Jan. 11, 2018, 9:51 p.m., Andrew Sherman wrote:
> > common/src/java/org/apache/hadoop/hive/common/LogUtils.java
> > Lines 259 (patched)
> > 
> >
> > Does deleteAppender() call stop() internally? If so then can we delete 
> > the previous call to stop the subordinateAppender?
> 
> kalyan kumar kalvagadda wrote:
> You are right. deleteAppender internallu calls stop() on the appender. we 
> need to explicitly look-up and stop it. I will update this in my next patch.

Thanks


> On Jan. 11, 2018, 9:51 p.m., Andrew Sherman wrote:
> > itests/hive-unit/src/test/java/org/apache/hive/service/cli/operation/TestOperationLoggingLayout.java
> > Line 162 (original), 163 (patched)
> > 
> >
> > This code was to check the case described in 
> > https://issues.apache.org/jira/browse/HIVE-17826?focusedCommentId=16208636=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-16208636
> > I think this is no longer tested. Is that OK?
> 
> kalyan kumar kalvagadda wrote:
> I have couple of observations with the limited knowledge that I have. 
> Correct me if i'm wrong here.
> 
> 1. HIVE-17128: Code changes done as part of this jira makes sure that 
> log4j Appender is closed when operation is closed to avoid file descriptors.
> 2. HIVE-17826: Added HushableRandomAccessFileAppender which is a copy of 
> RandomAccessFileAppender but has a explicit check in append() method to see 
> if the appender is closed.
> 
>  I think issue reported in HIVE-17826 is seen only because the 
> RoutingAppender is not properly cleaned-up when the operation is stopped. If 
> we have the patch i submitted we may not see the issue reported in HIVE-17826 
> even with out the fix of adding HushableRandomAccessFileAppender.

OK


- Andrew


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65075/#review195255
---


On Jan. 11, 2018, 2:11 p.m., kalyan kumar kalvagadda wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65075/
> ---
> 
> (Updated Jan. 11, 2018, 2:11 p.m.)
> 
> 
> Review request for hive, Aihua Xu, Andrew Sherman, and Sergio Pena.
> 
> 
> Bugs: HIVE-18426
> https://issues.apache.org/jira/browse/HIVE-18426
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> Each new operation creates new entry in the ConcurrentMap in RoutingAppender 
> but when the operation ends, AppenderControl stored in the map is retrieved 
> and stopped but the entry in ConcurrentMap is never cleaned up.
> 
> 
> Diffs
> -
> 
>   common/src/java/org/apache/hadoop/hive/common/LogUtils.java 
> 0a3e0c72011951b6b1543352308bd51233c847fb 
>   
> itests/hive-unit/src/test/java/org/apache/hive/service/cli/operation/TestOperationLoggingLayout.java
>  8febe3e79ff892c54b696b6c6ef92f7026c46033 
> 
> 
> Diff: https://reviews.apache.org/r/65075/diff/1/
> 
> 
> Testing
> ---
> 
> Made sure that the new tests updated to verify this change are passing.
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

Re: Review Request 65075: HIVE-18426: Memory leak in RoutingAppender for every hive operation

2018-01-11 Thread kalyan kumar kalvagadda via Review Board 


> On Jan. 11, 2018, 9:51 p.m., Andrew Sherman wrote:
> > common/src/java/org/apache/hadoop/hive/common/LogUtils.java
> > Lines 259 (patched)
> > 
> >
> > Does deleteAppender() call stop() internally? If so then can we delete 
> > the previous call to stop the subordinateAppender?

You are right. deleteAppender internallu calls stop() on the appender. we need 
to explicitly look-up and stop it. I will update this in my next patch.


> On Jan. 11, 2018, 9:51 p.m., Andrew Sherman wrote:
> > itests/hive-unit/src/test/java/org/apache/hive/service/cli/operation/TestOperationLoggingLayout.java
> > Line 162 (original), 163 (patched)
> > 
> >
> > This code was to check the case described in 
> > https://issues.apache.org/jira/browse/HIVE-17826?focusedCommentId=16208636=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-16208636
> > I think this is no longer tested. Is that OK?

I have couple of observations with the limited knowledge that I have. Correct 
me if i'm wrong here.

1. HIVE-17128: Code changes done as part of this jira makes sure that log4j 
Appender is closed when operation is closed to avoid file descriptors.
2. HIVE-17826: Added HushableRandomAccessFileAppender which is a copy of 
RandomAccessFileAppender but has a explicit check in append() method to see if 
the appender is closed.

 I think issue reported in HIVE-17826 is seen only because the RoutingAppender 
is not properly cleaned-up when the operation is stopped. If we have the patch 
i submitted we may not see the issue reported in HIVE-17826 even with out the 
fix of adding HushableRandomAccessFileAppender.


- kalyan kumar


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65075/#review195255
---


On Jan. 11, 2018, 2:11 p.m., kalyan kumar kalvagadda wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65075/
> ---
> 
> (Updated Jan. 11, 2018, 2:11 p.m.)
> 
> 
> Review request for hive, Aihua Xu, Andrew Sherman, and Sergio Pena.
> 
> 
> Bugs: HIVE-18426
> https://issues.apache.org/jira/browse/HIVE-18426
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> Each new operation creates new entry in the ConcurrentMap in RoutingAppender 
> but when the operation ends, AppenderControl stored in the map is retrieved 
> and stopped but the entry in ConcurrentMap is never cleaned up.
> 
> 
> Diffs
> -
> 
>   common/src/java/org/apache/hadoop/hive/common/LogUtils.java 
> 0a3e0c72011951b6b1543352308bd51233c847fb 
>   
> itests/hive-unit/src/test/java/org/apache/hive/service/cli/operation/TestOperationLoggingLayout.java
>  8febe3e79ff892c54b696b6c6ef92f7026c46033 
> 
> 
> Diff: https://reviews.apache.org/r/65075/diff/1/
> 
> 
> Testing
> ---
> 
> Made sure that the new tests updated to verify this change are passing.
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

Re: Review Request 65075: HIVE-18426: Memory leak in RoutingAppender for every hive operation

2018-01-11 Thread Andrew Sherman via Review Board 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65075/#review195255
---




common/src/java/org/apache/hadoop/hive/common/LogUtils.java
Lines 259 (patched)


Does deleteAppender() call stop() internally? If so then can we delete the 
previous call to stop the subordinateAppender?



itests/hive-unit/src/test/java/org/apache/hive/service/cli/operation/TestOperationLoggingLayout.java
Line 162 (original), 163 (patched)


This code was to check the case described in 
https://issues.apache.org/jira/browse/HIVE-17826?focusedCommentId=16208636=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-16208636
I think this is no longer tested. Is that OK?


- Andrew Sherman


On Jan. 11, 2018, 2:11 p.m., kalyan kumar kalvagadda wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65075/
> ---
> 
> (Updated Jan. 11, 2018, 2:11 p.m.)
> 
> 
> Review request for hive, Aihua Xu, Andrew Sherman, and Sergio Pena.
> 
> 
> Bugs: HIVE-18426
> https://issues.apache.org/jira/browse/HIVE-18426
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> Each new operation creates new entry in the ConcurrentMap in RoutingAppender 
> but when the operation ends, AppenderControl stored in the map is retrieved 
> and stopped but the entry in ConcurrentMap is never cleaned up.
> 
> 
> Diffs
> -
> 
>   common/src/java/org/apache/hadoop/hive/common/LogUtils.java 
> 0a3e0c72011951b6b1543352308bd51233c847fb 
>   
> itests/hive-unit/src/test/java/org/apache/hive/service/cli/operation/TestOperationLoggingLayout.java
>  8febe3e79ff892c54b696b6c6ef92f7026c46033 
> 
> 
> Diff: https://reviews.apache.org/r/65075/diff/1/
> 
> 
> Testing
> ---
> 
> Made sure that the new tests updated to verify this change are passing.
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

Review Request 65075: HIVE-18426: Memory leak in RoutingAppender for every hive operation

2018-01-11 Thread kalyan kumar kalvagadda via Review Board 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65075/
---

Review request for hive, Aihua Xu and Andrew Sherman.


Bugs: HIVE-18426
https://issues.apache.org/jira/browse/HIVE-18426


Repository: hive-git


Description
---

Each new operation creates new entry in the ConcurrentMap in RoutingAppender 
but when the operation ends, AppenderControl stored in the map is retrieved and 
stopped but the entry in ConcurrentMap is never cleaned up.


Diffs
-

  common/src/java/org/apache/hadoop/hive/common/LogUtils.java 
0a3e0c72011951b6b1543352308bd51233c847fb 
  
itests/hive-unit/src/test/java/org/apache/hive/service/cli/operation/TestOperationLoggingLayout.java
 8febe3e79ff892c54b696b6c6ef92f7026c46033 


Diff: https://reviews.apache.org/r/65075/diff/1/


Testing
---

Made sure that the new tests updated to verify this change is passing.


Thanks,

kalyan kumar kalvagadda