ambari-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Ajit Kumar" <ajit.ku...@hortonworks.com>
Subject Re: Review Request 42339: Provide CRUD support for admin-settings
Date Thu, 21 Jan 2016 04:40:46 GMT


> On Jan. 21, 2016, 4:27 a.m., Nahappan Somasundaram wrote:
> > ambari-server/src/test/java/org/apache/ambari/server/controller/internal/AdminSettingResourceProviderTest.java,
line 1
> > <https://reviews.apache.org/r/42339/diff/1/?file=1204061#file1204061line1>
> >
> >     Some comments on what each of the test methods do will help when debugging.

Each method name is self explanatory what the test method is doing like deleteResource/updateResource.


> On Jan. 21, 2016, 4:27 a.m., Nahappan Somasundaram wrote:
> > ambari-server/src/main/java/org/apache/ambari/server/api/resources/ResourceInstanceFactoryImpl.java,
line 41
> > <https://reviews.apache.org/r/42339/diff/1/?file=1204050#file1204050line41>
> >
> >     To reduce noise, it is recommended that changes for refactoring, cleanup, style
changes, etc., which do not affect the feature have a separate JIRA as a prep work.

I agree with you, if there are changes not related to JIRA, there is lot more noise. If there
are lot of clean up across many files, generally I publish first review with clean up changes
and then relevant changes so that reviewers can do a diff and look at clean up or actual changes,
the way they want. In this case changes were very minimal and filing a jira is too much overhead,
I just put it altogether.


> On Jan. 21, 2016, 4:27 a.m., Nahappan Somasundaram wrote:
> > ambari-server/src/main/java/org/apache/ambari/server/api/resources/ResourceInstanceFactoryImpl.java,
line 85
> > <https://reviews.apache.org/r/42339/diff/1/?file=1204050#file1204050line85>
> >
> >     Not really a fan of multiple return statements, unless that is the coding convention
in Ambari.

There is no ambari convention in this regard. There are many files which have one or other
pattern. I will revert this change.


- Ajit


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


On Jan. 21, 2016, 12:04 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, 12:04 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