ambari-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Alejandro Fernandez" <afernan...@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 18:54:43 GMT


> On Nov. 24, 2015, 6: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?
> 
> Sebastian Toader wrote:
>     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?
> 
> Dmitro Lisnichenko wrote:
>     I think this approach may break some stack scripts (params.py) at agent that check
property existence and not property value.
> 
> Sebastian Toader wrote:
>     Before this was 
>     
>     String value = properties.get(property).toString();
>     
>     propertiesMap.put(propertyName, value);
>     } catch (Exception e) {
>         LOG.debug(String.format("Error handling configuration property, name = %s", property),
e);
>        // do nothing
>     }
>     
>     and this was throwing NPE (which was silently logged) so properties with null values
never got added to propertiesMap. To me this means that properties with null values never
made it down to params.py, only properties with values. In fact form the stack defintion is
looks to me that we never had properties with null value before, these have been recently
introduced.
>     
>     Do you think it is safer to just add the null value to the map or simply ignore those
properties?

I vote to keep the behavior consistent; if the value is null, don't add the property.


- Alejandro


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


On Nov. 25, 2015, 11:13 a.m., Sebastian Toader wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40649/
> -----------------------------------------------------------
> 
> (Updated Nov. 25, 2015, 11:13 a.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
0e3e7b8 
>   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