----------------------------------------------------------- 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 > >
