Re: Review Request 71784: HiveProtoLoggingHook might consume lots of memory

[Harish Jaiprakash via Review Board]

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


Ship it!




LGTM.

- Harish Jaiprakash


On Nov. 21, 2019, 3:10 p.m., Attila Magyar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71784/
> ---
> 
> (Updated Nov. 21, 2019, 3:10 p.m.)
> 
> 
> Review request for hive, Laszlo Bodor, Harish Jaiprakash, Mustafa Iman, and 
> Panos Garefalakis.
> 
> 
> Bugs: HIVE-22514
> https://issues.apache.org/jira/browse/HIVE-22514
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> HiveProtoLoggingHook uses a ScheduledThreadPoolExecutor to submit writer 
> tasks and to periodically handle rollover. The builtin 
> ScheduledThreadPoolExecutor uses a unbounded queue which cannot be replaced 
> from the outside. If log events are generated at a very fast rate this queue 
> can grow large.
> 
> Since ScheduledThreadPoolExecutor does not support changing the default 
> unbounded queue to a bounded one, the queue capacity is checked manually by 
> the patch.
> 
> 
> Diffs
> -
> 
>   common/src/java/org/apache/hadoop/hive/conf/HiveConf.java a7687d59004 
>   ql/src/java/org/apache/hadoop/hive/ql/hooks/HiveProtoLoggingHook.java 
> 8eab54859bf 
>   ql/src/test/org/apache/hadoop/hive/ql/hooks/TestHiveProtoLoggingHook.java 
> 450a0b544d6 
> 
> 
> Diff: https://reviews.apache.org/r/71784/diff/2/
> 
> 
> Testing
> ---
> 
> unittest
> 
> 
> Thanks,
> 
> Attila Magyar
> 
>

Re: Review Request 71784: HiveProtoLoggingHook might consume lots of memory

[Harish Jaiprakash via Review Board]

> On Nov. 20, 2019, 7:32 a.m., Harish Jaiprakash wrote:
> > ql/src/test/org/apache/hadoop/hive/ql/hooks/TestHiveProtoLoggingHook.java
> > Lines 176 (patched)
> > 
> >
> > We expect the dequeue to have not happened by this time. There is no 
> > guarantee, since its another thread. Can we atleast add a comment that this 
> > test can fail intermittently?
> 
> Attila Magyar wrote:
> I guess this effects the existing tests as well, right? However I don't 
> remember seeing any of those faling. Maybe because we're calling the 
> shutdown() on the evtLogger. According to its javadoc it waits for already 
> submitted task to complete.

The existing tests do not rely on order of execution between threads. But this 
test relys on the fact that the ExecutorService thread is not scheduled before 
the 4 sumbits to the executor service are executed and hence can cause 
intermittent failures. The only way to fix it would be to expose the underlying 
executor service and submit a task which will block the thread. And do the rest 
of the calls.


- Harish


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


On Nov. 19, 2019, 9:13 p.m., Attila Magyar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71784/
> ---
> 
> (Updated Nov. 19, 2019, 9:13 p.m.)
> 
> 
> Review request for hive, Laszlo Bodor, Harish Jaiprakash, Mustafa Iman, and 
> Panos Garefalakis.
> 
> 
> Bugs: HIVE-22514
> https://issues.apache.org/jira/browse/HIVE-22514
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> HiveProtoLoggingHook uses a ScheduledThreadPoolExecutor to submit writer 
> tasks and to periodically handle rollover. The builtin 
> ScheduledThreadPoolExecutor uses a unbounded queue which cannot be replaced 
> from the outside. If log events are generated at a very fast rate this queue 
> can grow large.
> 
> Since ScheduledThreadPoolExecutor does not support changing the default 
> unbounded queue to a bounded one, the queue capacity is checked manually by 
> the patch.
> 
> 
> Diffs
> -
> 
>   common/src/java/org/apache/hadoop/hive/conf/HiveConf.java a7687d59004 
>   ql/src/java/org/apache/hadoop/hive/ql/hooks/HiveProtoLoggingHook.java 
> 8eab54859bf 
>   ql/src/test/org/apache/hadoop/hive/ql/hooks/TestHiveProtoLoggingHook.java 
> 450a0b544d6 
> 
> 
> Diff: https://reviews.apache.org/r/71784/diff/1/
> 
> 
> Testing
> ---
> 
> unittest
> 
> 
> Thanks,
> 
> Attila Magyar
> 
>