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