Return-Path: 
 <dev-return-96521-apmail-ambari-dev-archive=ambari.apache.org@ambari.apache.org>
X-Original-To: apmail-ambari-dev-archive@www.apache.org
Delivered-To: apmail-ambari-dev-archive@www.apache.org
Received: from mail.apache.org (hermes.apache.org [140.211.11.3])
	by minotaur.apache.org (Postfix) with SMTP id BC24E1861F
	for <apmail-ambari-dev-archive@www.apache.org>;
 Wed, 25 Nov 2015 18:54:50 +0000 (UTC)
Received: (qmail 40071 invoked by uid 500); 25 Nov 2015 18:54:45 -0000
Delivered-To: apmail-ambari-dev-archive@ambari.apache.org
Received: (qmail 40034 invoked by uid 500); 25 Nov 2015 18:54:45 -0000
Mailing-List: contact dev-help@ambari.apache.org; run by ezmlm
Precedence: bulk
List-Help: <mailto:dev-help@ambari.apache.org>
List-Unsubscribe: <mailto:dev-unsubscribe@ambari.apache.org>
List-Post: <mailto:dev@ambari.apache.org>
List-Id: <dev.ambari.apache.org>
Reply-To: dev@ambari.apache.org
Delivered-To: mailing list dev@ambari.apache.org
Received: (qmail 39974 invoked by uid 99); 25 Nov 2015 18:54:45 -0000
Received: from reviews-vm.apache.org (HELO reviews.apache.org) (140.211.11.40)
    by apache.org (qpsmtpd/0.29) with ESMTP; Wed, 25 Nov 2015 18:54:45 +0000
Received: from reviews.apache.org (localhost [127.0.0.1])
	by reviews.apache.org (Postfix) with ESMTP id CAAC72E4557;
	Wed, 25 Nov 2015 18:54:43 +0000 (UTC)
Content-Type: multipart/alternative;
 boundary="===============8421437380336460011=="
MIME-Version: 1.0
Subject: Re: Review Request 40649: Update cluster fails with NPE if the
 request
 contains null configuration property values
From: "Alejandro Fernandez" <afernandez@hortonworks.com>
To: "Alejandro Fernandez" <afernandez@hortonworks.com>,
 "Sumit Mohanty" <smohanty@hortonworks.com>,
 "Dmitro Lisnichenko" <dlysnichenko@hortonworks.com>
Cc: "Ambari" <dev@ambari.apache.org>,
 "Sebastian Toader" <stoader@hortonworks.com>
Date: Wed, 25 Nov 2015 18:54:43 -0000
Message-ID: <20151125185443.24387.59877@reviews.apache.org>
X-ReviewBoard-URL: https://reviews.apache.org/
Auto-Submitted: auto-generated
Sender: "Alejandro Fernandez" <noreply@reviews.apache.org>
X-ReviewGroup: Ambari
X-Auto-Response-Suppress: DR, RN, OOF, AutoReply
X-ReviewRequest-URL: https://reviews.apache.org/r/40649/
X-Sender: "Alejandro Fernandez" <noreply@reviews.apache.org>
References: <20151124182211.24387.7974@reviews.apache.org>
In-Reply-To: <20151124182211.24387.7974@reviews.apache.org>
Reply-To: "Alejandro Fernandez" <afernandez@hortonworks.com>
X-ReviewRequest-Repository: ambari

--===============8421437380336460011==
MIME-Version: 1.0
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 7bit



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


--===============8421437380336460011==--