You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@ambari.apache.org by Nate Cole <nc...@hortonworks.com> on 2014/10/01 16:55:09 UTC
Re: Review Request 26184: Clean up JMXPropertyProvider hacks for STORM
metrics
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26184/#review55074
-----------------------------------------------------------
ambari-server/src/main/java/org/apache/ambari/server/controller/internal/ResourceImpl.java
<https://reviews.apache.org/r/26184/#comment95427>
Excessive. Just use a ConcurrentHashMap. A TreeMap is just wasteful here since propertiesMap is just the Map interface.
ambari-server/src/main/java/org/apache/ambari/server/controller/internal/ResourceImpl.java
<https://reviews.apache.org/r/26184/#comment95428>
ConcurrentHashMap instead.
ambari-server/src/main/java/org/apache/ambari/server/controller/internal/ResourceImpl.java
<https://reviews.apache.org/r/26184/#comment95429>
Does the inner map really need to be synchronized?
ambari-server/src/main/java/org/apache/ambari/server/controller/internal/StackDefinedPropertyProvider.java
<https://reviews.apache.org/r/26184/#comment95430>
Javadoc no longer matches.
ambari-server/src/main/java/org/apache/ambari/server/controller/internal/StackDefinedPropertyProvider.java
<https://reviews.apache.org/r/26184/#comment95435>
This will break other people that have already used the old ctor.
ambari-server/src/main/java/org/apache/ambari/server/controller/internal/StackDefinedPropertyProvider.java
<https://reviews.apache.org/r/26184/#comment95431>
These changes are going to make it exceedingly difficult to make their own providers and specify them in json. This loses the spirit of that functionality. The internal implementations can have setters, but this ctor is too complex.
ambari-server/src/main/java/org/apache/ambari/server/controller/metrics/RestMetricsPropertyProvider.java
<https://reviews.apache.org/r/26184/#comment95436>
injector.injectMembers(this) will be helpful
ambari-server/src/main/java/org/apache/ambari/server/controller/metrics/RestMetricsPropertyProvider.java
<https://reviews.apache.org/r/26184/#comment95432>
You aren't actually parsing here, so a NumberFormatException can never be thrown.
ambari-server/src/main/java/org/apache/ambari/server/controller/metrics/RestMetricsPropertyProvider.java
<https://reviews.apache.org/r/26184/#comment95437>
This can return null. May want to initialize to DEFAULT_PROTOCOL then only check for an override.
ambari-server/src/main/java/org/apache/ambari/server/controller/metrics/RestMetricsPropertyProvider.java
<https://reviews.apache.org/r/26184/#comment95433>
Prefer Collections.emptySet() here
ambari-server/src/test/java/org/apache/ambari/server/controller/internal/StackDefinedPropertyProviderTest.java
<https://reviews.apache.org/r/26184/#comment95438>
Values are now strings instead of int? That will make previous consumers unhappy
- Nate Cole
On Sept. 30, 2014, 3:35 p.m., Andrew Onischuk wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26184/
> -----------------------------------------------------------
>
> (Updated Sept. 30, 2014, 3:35 p.m.)
>
>
> Review request for Ambari, Dmitro Lisnichenko and Nate Cole.
>
>
> Bugs: AMBARI-4881
> https://issues.apache.org/jira/browse/AMBARI-4881
>
>
> Repository: ambari
>
>
> Description
> -------
>
> Clean up a JMXPropertyProvider hacks for STORM metrics
> Also include comments from <https://reviews.apache.org/r/18521>
>
>
> Diffs
> -----
>
> ambari-server/src/main/java/org/apache/ambari/server/controller/internal/AbstractProviderModule.java fc60210
> ambari-server/src/main/java/org/apache/ambari/server/controller/internal/ResourceImpl.java 15fb961
> ambari-server/src/main/java/org/apache/ambari/server/controller/internal/StackDefinedPropertyProvider.java 51c7565
> ambari-server/src/main/java/org/apache/ambari/server/controller/jmx/JMXHostProvider.java 12f3725
> ambari-server/src/main/java/org/apache/ambari/server/controller/jmx/JMXPropertyProvider.java ca016f5
> ambari-server/src/main/java/org/apache/ambari/server/controller/metrics/MetricsHostProvider.java PRE-CREATION
> ambari-server/src/main/java/org/apache/ambari/server/controller/metrics/MetricsProvider.java PRE-CREATION
> ambari-server/src/main/java/org/apache/ambari/server/controller/metrics/RestMetricsPropertyProvider.java PRE-CREATION
> ambari-server/src/main/resources/stacks/HDP/2.1/services/STORM/metrics.json 83e27d1
> ambari-server/src/test/java/org/apache/ambari/server/controller/internal/StackDefinedPropertyProviderTest.java 2a086ae
> ambari-server/src/test/java/org/apache/ambari/server/controller/jmx/JMXPropertyProviderTest.java 24734f8
> ambari-server/src/test/java/org/apache/ambari/server/controller/metrics/JMXPropertyProviderTest.java PRE-CREATION
> ambari-server/src/test/resources/stacks/HDP/2.1.1/services/STORM/metrics.json 59bec39
>
> Diff: https://reviews.apache.org/r/26184/diff/
>
>
> Testing
> -------
>
> mvn clean test
>
>
> Thanks,
>
> Andrew Onischuk
>
>