ambari-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Nate Cole" <nc...@hortonworks.com>
Subject Re: Review Request 42339: Provide CRUD support for admin-settings
Date Thu, 21 Jan 2016 14:01:33 GMT

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



ambari-server/src/main/java/org/apache/ambari/server/controller/internal/AdminSettingResourceProvider.java
(line 123)
<https://reviews.apache.org/r/42339/#comment176604>

    Any reason this needs to be a LinkedList over an ArrayList?



ambari-server/src/main/java/org/apache/ambari/server/controller/internal/AdminSettingResourceProvider.java
(line 133)
<https://reviews.apache.org/r/42339/#comment176603>

    Prefer String.format() here.



ambari-server/src/main/java/org/apache/ambari/server/controller/internal/AdminSettingResourceProvider.java
(line 137)
<https://reviews.apache.org/r/42339/#comment176605>

    Will this end up duplicating an item in the entities list?  Say the first property map
has the name, then the code above adds that entity to the list.  The loop runs again for the
2nd property Map - if the name property isn't there, then dao.findAll() will add all entities.
 Would prefer a different mechanism than this.



ambari-server/src/main/java/org/apache/ambari/server/controller/internal/AdminSettingResourceProvider.java
(line 160)
<https://reviews.apache.org/r/42339/#comment176607>

    Will end up with NPE if the map doesn't contain the name



ambari-server/src/main/java/org/apache/ambari/server/controller/internal/AdminSettingResourceProvider.java
(line 200)
<https://reviews.apache.org/r/42339/#comment176608>

    May need a bit more detail here.  If this gets thrown to the client there's no information
what the user has done wrong, or why it's wrong.



ambari-server/src/main/java/org/apache/ambari/server/controller/internal/AdminSettingResourceProvider.java
(line 220)
<https://reviews.apache.org/r/42339/#comment176609>

    Should use setResourceProperty() method for these.



ambari-server/src/main/java/org/apache/ambari/server/orm/entities/AdminSettingEntity.java
(line 43)
<https://reviews.apache.org/r/42339/#comment176612>

    Add javadoc, even if it's simplistic



ambari-server/src/main/java/org/apache/ambari/server/orm/entities/AdminSettingEntity.java
(lines 76 - 84)
<https://reviews.apache.org/r/42339/#comment176610>

    Remove this constructor.  It's only used for a test and doesn't appear to be a business
rule.  And if it were, use a clone() method instead.



ambari-server/src/main/java/org/apache/ambari/server/orm/entities/AdminSettingEntity.java
(lines 134 - 136)
<https://reviews.apache.org/r/42339/#comment176613>

    reflection based stuff can be slow.  Prefer using member variables for consistency.



ambari-server/src/main/java/org/apache/ambari/server/orm/entities/AdminSettingEntity.java
(lines 138 - 141)
<https://reviews.apache.org/r/42339/#comment176614>

    reflection based stuff can be slow.  Prefer using member variables for consistency.



ambari-server/src/main/resources/key_properties.json (lines 151 - 153)
<https://reviews.apache.org/r/42339/#comment176615>

    Use of this file is deprecated.  Define key properties in ResourceProvider directly.



ambari-server/src/main/resources/properties.json (lines 461 - 468)
<https://reviews.apache.org/r/42339/#comment176616>

    Use of this file is deprecated.  Define key properties in ResourceProvider directly.


- Nate Cole


On Jan. 21, 2016, 1:23 a.m., Ajit Kumar wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42339/
> -----------------------------------------------------------
> 
> (Updated Jan. 21, 2016, 1:23 a.m.)
> 
> 
> Review request for Ambari, Alejandro Fernandez, Jayush Luniya, Nate Cole, Nahappan Somasundaram,
Sumit Mohanty, and Sid Wagle.
> 
> 
> Bugs: AMBARI-14750
>     https://issues.apache.org/jira/browse/AMBARI-14750
> 
> 
> Repository: ambari
> 
> 
> Description
> -------
> 
> Provide CRUD support for admin-settings
> 
> 
> Diffs
> -----
> 
>   ambari-server/src/main/java/org/apache/ambari/server/api/resources/ResourceInstanceFactoryImpl.java
c7c5d61b70ea38f22861519783733e9b9ebd415d 
>   ambari-server/src/main/java/org/apache/ambari/server/api/services/AdminSettingService.java
PRE-CREATION 
>   ambari-server/src/main/java/org/apache/ambari/server/controller/internal/AbstractControllerResourceProvider.java
cce3764b36e4d8a4ca0762329c941405b0966fc2 
>   ambari-server/src/main/java/org/apache/ambari/server/controller/internal/AdminSettingResourceProvider.java
PRE-CREATION 
>   ambari-server/src/main/java/org/apache/ambari/server/controller/internal/DefaultProviderModule.java
3801cc3da56769b2dfc5ca1ceb22a8aa95eab63f 
>   ambari-server/src/main/java/org/apache/ambari/server/controller/spi/Resource.java e367afe4e319a8de441d201d619da4f20b48dcdb

>   ambari-server/src/main/java/org/apache/ambari/server/orm/dao/AdminSettingDAO.java PRE-CREATION

>   ambari-server/src/main/java/org/apache/ambari/server/orm/entities/AdminSettingEntity.java
PRE-CREATION 
>   ambari-server/src/main/resources/META-INF/persistence.xml 3979bc0782840284678d5e2d6f5af797d011aa18

>   ambari-server/src/main/resources/key_properties.json 46a6cf9b3f1d171ed8250df787cb671cce426c02

>   ambari-server/src/main/resources/properties.json 4052ad213eeb5da8475c6e80d9b404fada256fa9

>   ambari-server/src/test/java/org/apache/ambari/server/controller/internal/AdminSettingResourceProviderTest.java
PRE-CREATION 
>   ambari-server/src/test/java/org/apache/ambari/server/orm/dao/AdminSettingDAOTest.java
PRE-CREATION 
>   ambari-server/src/test/java/org/apache/ambari/server/orm/entities/AdminSettingEntityTest.java
PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/42339/diff/
> 
> 
> Testing
> -------
> 
> Tested API through curl
> unit tests
> 
> [INFO] ------------------------------------------------------------------------
> [INFO] Reactor Summary:
> [INFO]
> [INFO] Ambari Main ....................................... SUCCESS [9.890s]
> [INFO] Apache Ambari Project POM ......................... SUCCESS [0.060s]
> [INFO] Ambari Web ........................................ SUCCESS [2:15.144s]
> [INFO] Ambari Views ...................................... SUCCESS [3.885s]
> [INFO] Ambari Admin View ................................. SUCCESS [11.634s]
> [INFO] ambari-metrics .................................... SUCCESS [0.388s]
> [INFO] Ambari Metrics Common ............................. SUCCESS [2.729s]
> [INFO] Ambari Metrics Hadoop Sink ........................ SUCCESS [5.239s]
> [INFO] Ambari Metrics Flume Sink ......................... SUCCESS [2.931s]
> [INFO] Ambari Metrics Kafka Sink ......................... SUCCESS [4.818s]
> [INFO] Ambari Metrics Storm Sink ......................... SUCCESS [2.567s]
> [INFO] Ambari Metrics Collector .......................... SUCCESS [1:00.778s]
> [INFO] Ambari Metrics Monitor ............................ SUCCESS [3.318s]
> [INFO] Ambari Metrics Assembly ........................... SUCCESS [59.382s]
> [INFO] Ambari Server ..................................... SUCCESS [1:03:30.478s]
> [INFO] Ambari Functional Tests ........................... SUCCESS [1:57.459s]
> [INFO] Ambari Agent ...................................... SUCCESS [13.973s]
> [INFO] Ambari Client ..................................... SUCCESS [0.041s]
> [INFO] Ambari Python Client .............................. SUCCESS [2.306s]
> [INFO] Ambari Groovy Client .............................. SUCCESS [12.051s]
> [INFO] Ambari Shell ...................................... SUCCESS [0.033s]
> [INFO] Ambari Python Shell ............................... SUCCESS [0.863s]
> [INFO] Ambari Groovy Shell ............................... SUCCESS [9.254s]
> [INFO] ------------------------------------------------------------------------
> [INFO] BUILD SUCCESS
> [INFO] ------------------------------------------------------------------------
> [INFO] Total time: 1:11:12.017s
> [INFO] Finished at: Wed Jan 20 16:02:44 PST 2016
> [INFO] Final Memory: 223M/1079M
> [INFO] ------------------------------------------------------------------------
> 
> 
> Thanks,
> 
> Ajit Kumar
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message