ambari-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Sebastian Toader" <stoa...@hortonworks.com>
Subject Re: Review Request 40649: Update cluster fails with NPE if the request contains null configuration property values
Date Wed, 25 Nov 2015 09:33:41 GMT


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


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