Re: Review Request 48508: AMBARI-17149 : HadoopTimelineMetricsSink from AMS prevents HBase RS process shutdown

[Dmytro Sen]

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


Ship it!




Ship It!

- Dmytro Sen


On Июнь 10, 2016, полночь, Aravindan Vijayan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/48508/
> ---
> 
> (Updated Июнь 10, 2016, полночь)
> 
> 
> Review request for Ambari, Dmytro Sen, enis, and Sumit Mohanty.
> 
> 
> Bugs: AMBARI-17149
> https://issues.apache.org/jira/browse/AMBARI-17149
> 
> 
> Repository: ambari
> 
> 
> Description
> ---
> 
> HadoopTimelineMetricsSink has an ExecutorService thread which is not a daemon 
> thread. 
> 
> The executor is not shutdown, and thread is not interrupted while the sink 
> stops. This causes region server process to hang although the rest of the 
> threads have all exited. The new thread should be marked as a daemon thread, 
> and also call shutdown() or shutdownNow() on the executor in sink close.
> 
> 
> Diffs
> -
> 
>   
> ambari-metrics/ambari-metrics-hadoop-sink/src/main/java/org/apache/hadoop/metrics2/sink/timeline/HadoopTimelineMetricsSink.java
>  0580088 
> 
> Diff: https://reviews.apache.org/r/48508/diff/
> 
> 
> Testing
> ---
> 
> Manually tested. Unit tests pass.
> 
> 
> Thanks,
> 
> Aravindan Vijayan
> 
>

Re: Review Request 48508: AMBARI-17149 : HadoopTimelineMetricsSink from AMS prevents HBase RS process shutdown

[enis]

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


Ship it!




Ship It!

- enis


On June 10, 2016, midnight, Aravindan Vijayan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/48508/
> ---
> 
> (Updated June 10, 2016, midnight)
> 
> 
> Review request for Ambari, Dmytro Sen, enis, and Sumit Mohanty.
> 
> 
> Bugs: AMBARI-17149
> https://issues.apache.org/jira/browse/AMBARI-17149
> 
> 
> Repository: ambari
> 
> 
> Description
> ---
> 
> HadoopTimelineMetricsSink has an ExecutorService thread which is not a daemon 
> thread. 
> 
> The executor is not shutdown, and thread is not interrupted while the sink 
> stops. This causes region server process to hang although the rest of the 
> threads have all exited. The new thread should be marked as a daemon thread, 
> and also call shutdown() or shutdownNow() on the executor in sink close.
> 
> 
> Diffs
> -
> 
>   
> ambari-metrics/ambari-metrics-hadoop-sink/src/main/java/org/apache/hadoop/metrics2/sink/timeline/HadoopTimelineMetricsSink.java
>  0580088 
> 
> Diff: https://reviews.apache.org/r/48508/diff/
> 
> 
> Testing
> ---
> 
> Manually tested. Unit tests pass.
> 
> 
> Thanks,
> 
> Aravindan Vijayan
> 
>

Re: Review Request 48508: AMBARI-17149 : HadoopTimelineMetricsSink from AMS prevents HBase RS process shutdown

[Aravindan Vijayan]

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

(Updated June 10, 2016, midnight)


Review request for Ambari, Dmytro Sen and Sumit Mohanty.


Changes
---

Fixed review issues.


Bugs: AMBARI-17149
https://issues.apache.org/jira/browse/AMBARI-17149


Repository: ambari


Description
---

HadoopTimelineMetricsSink has an ExecutorService thread which is not a daemon 
thread. 

The executor is not shutdown, and thread is not interrupted while the sink 
stops. This causes region server process to hang although the rest of the 
threads have all exited. The new thread should be marked as a daemon thread, 
and also call shutdown() or shutdownNow() on the executor in sink close.


Diffs (updated)
-

  
ambari-metrics/ambari-metrics-hadoop-sink/src/main/java/org/apache/hadoop/metrics2/sink/timeline/HadoopTimelineMetricsSink.java
 0580088 

Diff: https://reviews.apache.org/r/48508/diff/


Testing
---

Manually tested. Unit tests pass.


Thanks,

Aravindan Vijayan

Re: Review Request 48508: AMBARI-17149 : HadoopTimelineMetricsSink from AMS prevents HBase RS process shutdown

[enis]

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




ambari-metrics/ambari-metrics-hadoop-sink/src/main/java/org/apache/hadoop/metrics2/sink/timeline/HadoopTimelineMetricsSink.java
 (line 406)


ExecutorService is already a single threaded executor. We should not pass a 
Thread to it. Instead as before, we should pass a Runnuble, but make the 
threads from the executor service to be daemon threads. 

Alternatively, we can get rid of the executor alltogether, and only do the 
regular old Threads.


- enis


On June 9, 2016, 8:40 p.m., Aravindan Vijayan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/48508/
> ---
> 
> (Updated June 9, 2016, 8:40 p.m.)
> 
> 
> Review request for Ambari, Dmytro Sen and Sumit Mohanty.
> 
> 
> Bugs: AMBARI-17149
> https://issues.apache.org/jira/browse/AMBARI-17149
> 
> 
> Repository: ambari
> 
> 
> Description
> ---
> 
> HadoopTimelineMetricsSink has an ExecutorService thread which is not a daemon 
> thread. 
> 
> The executor is not shutdown, and thread is not interrupted while the sink 
> stops. This causes region server process to hang although the rest of the 
> threads have all exited. The new thread should be marked as a daemon thread, 
> and also call shutdown() or shutdownNow() on the executor in sink close.
> 
> 
> Diffs
> -
> 
>   
> ambari-metrics/ambari-metrics-hadoop-sink/src/main/java/org/apache/hadoop/metrics2/sink/timeline/HadoopTimelineMetricsSink.java
>  0580088 
> 
> Diff: https://reviews.apache.org/r/48508/diff/
> 
> 
> Testing
> ---
> 
> Manually tested. Unit tests pass.
> 
> 
> Thanks,
> 
> Aravindan Vijayan
> 
>

Re: Review Request 48508: AMBARI-17149 : HadoopTimelineMetricsSink from AMS prevents HBase RS process shutdown

[Sumit Mohanty]

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


Ship it!




Ship It!

- Sumit Mohanty


On June 9, 2016, 8:40 p.m., Aravindan Vijayan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/48508/
> ---
> 
> (Updated June 9, 2016, 8:40 p.m.)
> 
> 
> Review request for Ambari, Dmytro Sen and Sumit Mohanty.
> 
> 
> Bugs: AMBARI-17149
> https://issues.apache.org/jira/browse/AMBARI-17149
> 
> 
> Repository: ambari
> 
> 
> Description
> ---
> 
> HadoopTimelineMetricsSink has an ExecutorService thread which is not a daemon 
> thread. 
> 
> The executor is not shutdown, and thread is not interrupted while the sink 
> stops. This causes region server process to hang although the rest of the 
> threads have all exited. The new thread should be marked as a daemon thread, 
> and also call shutdown() or shutdownNow() on the executor in sink close.
> 
> 
> Diffs
> -
> 
>   
> ambari-metrics/ambari-metrics-hadoop-sink/src/main/java/org/apache/hadoop/metrics2/sink/timeline/HadoopTimelineMetricsSink.java
>  0580088 
> 
> Diff: https://reviews.apache.org/r/48508/diff/
> 
> 
> Testing
> ---
> 
> Manually tested. Unit tests pass.
> 
> 
> Thanks,
> 
> Aravindan Vijayan
> 
>

Review Request 48508: AMBARI-17149 : HadoopTimelineMetricsSink from AMS prevents HBase RS process shutdown

[Aravindan Vijayan]

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

Review request for Ambari, Dmytro Sen and Sumit Mohanty.


Bugs: AMBARI-17149
https://issues.apache.org/jira/browse/AMBARI-17149


Repository: ambari


Description
---

HadoopTimelineMetricsSink has an ExecutorService thread which is not a daemon 
thread. 

The executor is not shutdown, and thread is not interrupted while the sink 
stops. This causes region server process to hang although the rest of the 
threads have all exited. The new thread should be marked as a daemon thread, 
and also call shutdown() or shutdownNow() on the executor in sink close.


Diffs
-

  
ambari-metrics/ambari-metrics-hadoop-sink/src/main/java/org/apache/hadoop/metrics2/sink/timeline/HadoopTimelineMetricsSink.java
 0580088 

Diff: https://reviews.apache.org/r/48508/diff/


Testing
---

Manually tested. Unit tests pass.


Thanks,

Aravindan Vijayan