> On Nov. 24, 2015, 7:22 p.m., Alejandro Fernandez wrote: > > ambari-server/src/main/java/org/apache/ambari/server/controller/internal/StackAdvisorResourceProvider.java, > > line 270 > > <https://reviews.apache.org/r/40649/diff/1/?file=1138884#file1138884line270> > > > > Can you explain why this uses empty string if the prop is null?
The original implementation asumed that there is always a value for the prop. If the prop was null that caused NPE which was simply swallowed by the catch and logged. I looked at the stack definitions and properties either have a concrete value or empty string, so basically we need to be prepared for cases when no values are specified at all for a property (missing <value></value> element from xml file). As there is no guarantee that downstream where property values are used the code is guarded against NPE so I considered that is safer to use empty string as property value instead of searching through the code to find the places where property values are used and ensure that do not throw NPE in case of null reference values. Of course this may have a downside specifically if there is somewhere logic built on null reference property values. Are you aware of such logic in the code? - Sebastian ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/40649/#review107811 ----------------------------------------------------------- On Nov. 24, 2015, 4:26 p.m., Sebastian Toader wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/40649/ > ----------------------------------------------------------- > > (Updated Nov. 24, 2015, 4:26 p.m.) > > > Review request for Ambari, Alejandro Fernandez, Dmitro Lisnichenko, and Sumit > Mohanty. > > > Bugs: AMBARI-14040 > https://issues.apache.org/jira/browse/AMBARI-14040 > > > Repository: ambari > > > Description > ------- > > Upon update cluster request Ambari iterates through the passed in > configuration properties to verify if these differ from the existing cluster > configuration. The logic that iterates through the passed in properties is > not prepared for null property values. > > > Diffs > ----- > > > ambari-server/src/main/java/org/apache/ambari/server/controller/AmbariManagementControllerImpl.java > 7cb7f7d > > ambari-server/src/main/java/org/apache/ambari/server/controller/internal/StackAdvisorResourceProvider.java > fe3d006 > > ambari-server/src/test/java/org/apache/ambari/server/controller/AmbariManagementControllerImplTest.java > ca3ca36 > > ambari-server/src/test/java/org/apache/ambari/server/controller/internal/StackAdvisorResourceProviderTest.java > 8c5337b > > Diff: https://reviews.apache.org/r/40649/diff/ > > > Testing > ------- > > Manual testing: > 1. Deployed a cluster using wizzard than added services to the cluster > 2. Deployed a cluster using Blueprint than add new services to the cluster > > > Unit tests: > ---------------------------------------------------------------------- > Total run:802 > Total errors:0 > Total failures:0 > OK > StackAdvisor implementation for stack HDP1, version 2.0.6 was not found > Returning DefaultStackAdvisor implementation > StackAdvisor implementation for stack XYZ, version 1.0.0 was loaded > StackAdvisor implementation for stack XYZ, version 1.0.1 was loaded > Returning XYZ101StackAdvisor implementation > [INFO] > ------------------------------------------------------------------------ > [INFO] BUILD SUCCESS > [INFO] > ------------------------------------------------------------------------ > [INFO] Total time: 1:26:48.878s > [INFO] Finished at: Tue Nov 24 15:51:32 CET 2015 > [INFO] Final Memory: 39M/1612M > [INFO] > ------------------------------------------------------------------------ > > > Thanks, > > Sebastian Toader > >
