You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@ambari.apache.org by Andrew Onischuk <ao...@hortonworks.com> on 2014/09/30 20:09:45 UTC
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/
-----------------------------------------------------------
Review request for Ambari and Dmitro Lisnichenko.
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
Re: Review Request 26184: Clean up JMXPropertyProvider hacks for STORM
metrics
Posted by Dmitro Lisnichenko <dl...@hortonworks.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26184/#review54994
-----------------------------------------------------------
ambari-server/src/main/java/org/apache/ambari/server/controller/jmx/JMXPropertyProvider.java
<https://reviews.apache.org/r/26184/#comment95321>
Let's remove that
- Dmitro Lisnichenko
On Sept. 30, 2014, 6:16 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, 6:16 p.m.)
>
>
> Review request for Ambari and Dmitro Lisnichenko.
>
>
> 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
>
>
Re: Review Request 26184: Clean up JMXPropertyProvider hacks for STORM
metrics
Posted by Nate Cole <nc...@hortonworks.com>.
-----------------------------------------------------------
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
>
>
Re: Review Request 26184: Clean up JMXPropertyProvider hacks for STORM
metrics
Posted by Nate Cole <nc...@hortonworks.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26184/#review55202
-----------------------------------------------------------
Ship it!
Ship It!
- Nate Cole
On Oct. 2, 2014, 5:37 a.m., Andrew Onischuk wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26184/
> -----------------------------------------------------------
>
> (Updated Oct. 2, 2014, 5:37 a.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/main/resources/stacks/HDP/2.2/services/STORM/metrics.json PRE-CREATION
> 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
>
>
Re: Review Request 26184: Clean up JMXPropertyProvider hacks for STORM
metrics
Posted by Dmitro Lisnichenko <dl...@hortonworks.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26184/#review55203
-----------------------------------------------------------
Ship it!
Ship It!
- Dmitro Lisnichenko
On Oct. 2, 2014, 9:37 a.m., Andrew Onischuk wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26184/
> -----------------------------------------------------------
>
> (Updated Oct. 2, 2014, 9:37 a.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/main/resources/stacks/HDP/2.2/services/STORM/metrics.json PRE-CREATION
> 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
>
>
Re: Review Request 26184: Clean up JMXPropertyProvider hacks for STORM
metrics
Posted by Andrew Onischuk <ao...@hortonworks.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26184/
-----------------------------------------------------------
(Updated Oct. 2, 2014, 9:37 a.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 (updated)
-----
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/main/resources/stacks/HDP/2.2/services/STORM/metrics.json PRE-CREATION
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
Re: Review Request 26184: Clean up JMXPropertyProvider hacks for STORM
metrics
Posted by Andrew Onischuk <ao...@hortonworks.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26184/
-----------------------------------------------------------
(Updated Sept. 30, 2014, 7: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
Re: Review Request 26184: Clean up JMXPropertyProvider hacks for STORM
metrics
Posted by Andrew Onischuk <ao...@hortonworks.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26184/
-----------------------------------------------------------
(Updated Sept. 30, 2014, 6:16 p.m.)
Review request for Ambari and Dmitro Lisnichenko.
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