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