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




ambari-server/conf/unix/metrics.properties (line 24)
<https://reviews.apache.org/r/50995/#comment211791>

    We should call the source JvmMetricsSource instead of System since system 
also means OS level stats.



ambari-server/src/main/java/org/apache/ambari/server/metrics/system/impl/AmbariMetricSinkImpl.java
 (line 38)
<https://reviews.apache.org/r/50995/#comment211793>

    Add doc as to what form of URI is expected.
    This code xould break since there are not checks on input URI just appends.



ambari-server/src/main/java/org/apache/ambari/server/metrics/system/impl/AmbariMetricSinkImpl.java
 (line 42)
<https://reviews.apache.org/r/50995/#comment211794>

    Shouldn't this look something like Map<String, List<Number>> ?
    
    Seems as though there is no fan-out here. We collect more often than we 
publish, generally.



ambari-server/src/main/java/org/apache/ambari/server/metrics/system/impl/Configuration.java
 (line 46)
<https://reviews.apache.org/r/50995/#comment211799>

    Same classloader loads class, this seems very odd? You can use your own 
classloader instead.



ambari-server/src/main/java/org/apache/ambari/server/metrics/system/impl/MetricsServiceImpl.java
 (line 41)
<https://reviews.apache.org/r/50995/#comment211800>

    Explicit imports only.



ambari-server/src/main/java/org/apache/ambari/server/metrics/system/impl/MetricsServiceImpl.java
 (line 103)
<https://reviews.apache.org/r/50995/#comment211802>

    Null check ?



ambari-server/src/main/java/org/apache/ambari/server/metrics/system/impl/MetricsServiceImpl.java
 (line 104)
<https://reviews.apache.org/r/50995/#comment211801>

    formatting.



ambari-server/src/main/java/org/apache/ambari/server/metrics/system/impl/MetricsServiceImpl.java
 (line 105)
<https://reviews.apache.org/r/50995/#comment211804>

    Is null check needed?



ambari-server/src/main/java/org/apache/ambari/server/metrics/system/impl/MetricsServiceImpl.java
 (line 125)
<https://reviews.apache.org/r/50995/#comment211805>

    Why tmp_sources: missed naming convention here.



ambari-server/src/main/java/org/apache/ambari/server/metrics/system/impl/MetricsServiceImpl.java
 (line 148)
<https://reviews.apache.org/r/50995/#comment211806>

    This should say: Failing scheduling source instead of publish.



ambari-server/src/main/java/org/apache/ambari/server/metrics/system/impl/SystemMetricsSource.java
 (line 55)
<https://reviews.apache.org/r/50995/#comment211807>

    Why static?


- Sid Wagle


On Aug. 11, 2016, 6:21 p.m., Li-Wei Tseng wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50995/
> -----------------------------------------------------------
> 
> (Updated Aug. 11, 2016, 6:21 p.m.)
> 
> 
> Review request for Ambari, Aravindan Vijayan, Sumit Mohanty, Sid Wagle, and 
> Yusaku Sako.
> 
> 
> Bugs: AMBARI-17591
>     https://issues.apache.org/jira/browse/AMBARI-17591
> 
> 
> Repository: ambari
> 
> 
> Description
> -------
> 
> Created metrics service which has a JVM metrics source and a sink. The JVM 
> metrics source collect JVM metrics and pass it to the sink which then publish 
> the metrics to AMS
> 
> 
> Diffs
> -----
> 
>   ambari-server/conf/unix/metrics.properties PRE-CREATION 
>   ambari-server/pom.xml c2ee86d 
>   
> ambari-server/src/main/java/org/apache/ambari/server/controller/AmbariServer.java
>  5d0bb18 
>   
> ambari-server/src/main/java/org/apache/ambari/server/controller/ControllerModule.java
>  2bd7eff 
>   
> ambari-server/src/main/java/org/apache/ambari/server/metrics/system/AmbariMetricSink.java
>  PRE-CREATION 
>   
> ambari-server/src/main/java/org/apache/ambari/server/metrics/system/MetricsService.java
>  PRE-CREATION 
>   
> ambari-server/src/main/java/org/apache/ambari/server/metrics/system/MetricsSource.java
>  PRE-CREATION 
>   
> ambari-server/src/main/java/org/apache/ambari/server/metrics/system/impl/AbstractMetricsSource.java
>  PRE-CREATION 
>   
> ambari-server/src/main/java/org/apache/ambari/server/metrics/system/impl/AmbariMetricSinkImpl.java
>  PRE-CREATION 
>   
> ambari-server/src/main/java/org/apache/ambari/server/metrics/system/impl/Configuration.java
>  PRE-CREATION 
>   
> ambari-server/src/main/java/org/apache/ambari/server/metrics/system/impl/MetricsServiceImpl.java
>  PRE-CREATION 
>   
> ambari-server/src/main/java/org/apache/ambari/server/metrics/system/impl/SystemMetricsSource.java
>  PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/50995/diff/
> 
> 
> Testing
> -------
> 
> Manually tested it.
> 
> 
> Thanks,
> 
> Li-Wei Tseng
> 
>

Reply via email to