Re: Review Request 44916: StackAdvisor needs to set config map for ams-grafana-env to meet non-null expectations

2016-03-20 Thread Robert Levas 

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

(Updated March 16, 2016, 4:55 p.m.)


Review request for Ambari, Dmytro Sen, Jonathan Hurley, Nate Cole, and Sid 
Wagle.


Changes
---

Added NPE protection in Kerberos and Blueprint logic for recommended 
configrations.  Updated test case to provided null values for property maps.


Bugs: AMBARI-15442
https://issues.apache.org/jira/browse/AMBARI-15442


Repository: ambari


Description
---

The StackAdvisor needs to set a config map for the `ams-grafana-env`, even it 
is empty, to meet non-null expectations in Kerberos and Blueprint logic. 

To to this, the following line needs to be added to the StackAdvisor code:

#stacks/HDP/2.0.6/services/stack_advisor.py:473
```
putGrafanaProperty = self.putProperty(configurations, "ams-grafana-env", 
services)
```


Diffs (updated)
-

  
ambari-server/src/main/java/org/apache/ambari/server/api/services/stackadvisor/StackAdvisorBlueprintProcessor.java
 ea9fffb 
  
ambari-server/src/main/java/org/apache/ambari/server/controller/KerberosHelperImpl.java
 be6edc9 
  
ambari-server/src/main/java/org/apache/ambari/server/controller/internal/BlueprintConfigurationProcessor.java
 f5e7578 
  ambari-server/src/main/resources/stacks/HDP/2.0.6/services/stack_advisor.py 
8c18421 
  
ambari-server/src/test/java/org/apache/ambari/server/api/services/stackadvisor/StackAdvisorBlueprintProcessorTest.java
 c4e694b 
  
ambari-server/src/test/java/org/apache/ambari/server/controller/KerberosHelperTest.java
 f6027f3 
  
ambari-server/src/test/java/org/apache/ambari/server/controller/internal/BlueprintConfigurationProcessorTest.java
 68d5755 
  ambari-server/src/test/python/stacks/2.2/common/test_stack_advisor.py 9070c0a 

Diff: https://reviews.apache.org/r/44916/diff/


Testing
---

Manually tested in cluster Kerbrerized using the Ambari UI.


Thanks,

Robert Levas

Re: Review Request 44916: StackAdvisor needs to set config map for ams-grafana-env to meet non-null expectations

2016-03-19 Thread Sid Wagle 

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



Shouldn't we also have a non-null check in the backend code?

- Sid Wagle


On March 16, 2016, 4:59 p.m., Robert Levas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44916/
> ---
> 
> (Updated March 16, 2016, 4:59 p.m.)
> 
> 
> Review request for Ambari, Dmytro Sen, Jonathan Hurley, Nate Cole, and Sid 
> Wagle.
> 
> 
> Bugs: AMBARI-15442
> https://issues.apache.org/jira/browse/AMBARI-15442
> 
> 
> Repository: ambari
> 
> 
> Description
> ---
> 
> The StackAdvisor needs to set a config map for the `ams-grafana-env`, even it 
> is empty, to meet non-null expectations in Kerberos and Blueprint logic. 
> 
> To to this, the following line needs to be added to the StackAdvisor code:
> 
> #stacks/HDP/2.0.6/services/stack_advisor.py:473
> ```
> putGrafanaProperty = self.putProperty(configurations, "ams-grafana-env", 
> services)
> ```
> 
> 
> Diffs
> -
> 
>   ambari-server/src/main/resources/stacks/HDP/2.0.6/services/stack_advisor.py 
> 8c18421 
> 
> Diff: https://reviews.apache.org/r/44916/diff/
> 
> 
> Testing
> ---
> 
> Manually tested in cluster Kerbrerized using the Ambari UI.
> 
> 
> Thanks,
> 
> Robert Levas
> 
>

Re: Review Request 44916: StackAdvisor needs to set config map for ams-grafana-env to meet non-null expectations

2016-03-19 Thread Robert Levas 

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

(Updated March 17, 2016, 1:48 p.m.)


Review request for Ambari, Dmytro Sen, Jonathan Hurley, Nate Cole, and Sid 
Wagle.


Bugs: AMBARI-15442
https://issues.apache.org/jira/browse/AMBARI-15442


Repository: ambari


Description
---

The StackAdvisor needs to set a config map for the `ams-grafana-env`, even it 
is empty, to meet non-null expectations in Kerberos and Blueprint logic. 

To to this, the following line needs to be added to the StackAdvisor code:

#stacks/HDP/2.0.6/services/stack_advisor.py:473
```
putGrafanaProperty = self.putProperty(configurations, "ams-grafana-env", 
services)
```


Diffs (updated)
-

  
ambari-server/src/main/java/org/apache/ambari/server/api/services/stackadvisor/recommendations/RecommendationResponse.java
 4a66677 
  ambari-server/src/main/resources/stacks/HDP/2.0.6/services/stack_advisor.py 
54d6d6e 
  ambari-server/src/test/python/stacks/2.2/common/test_stack_advisor.py 59eff4d 

Diff: https://reviews.apache.org/r/44916/diff/


Testing
---

Manually tested in cluster Kerbrerized using the Ambari UI.


Thanks,

Robert Levas

Re: Review Request 44916: StackAdvisor needs to set config map for ams-grafana-env to meet non-null expectations

2016-03-19 Thread Jonathan Hurley 


> On March 16, 2016, 5:14 p.m., Jonathan Hurley wrote:
> > ambari-server/src/main/java/org/apache/ambari/server/api/services/stackadvisor/StackAdvisorBlueprintProcessor.java,
> >  line 224
> > 
> >
> > How can this ever be null? An empty config type!?
> 
> Robert Levas wrote:
> This is not config properties from a service config type, but updates 
> generated by the stack advisor. Up until now, this hasn't been null, but it 
> is possible based on the data the stack advisor returns.

My point was that you're adding a ton of null checks around this area of the 
code. It seems safer to maybe have the contract return an empty map? That way 
you don't have to constantly wrap this in an if-statement?


- Jonathan


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


On March 16, 2016, 4:55 p.m., Robert Levas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44916/
> ---
> 
> (Updated March 16, 2016, 4:55 p.m.)
> 
> 
> Review request for Ambari, Dmytro Sen, Jonathan Hurley, Nate Cole, and Sid 
> Wagle.
> 
> 
> Bugs: AMBARI-15442
> https://issues.apache.org/jira/browse/AMBARI-15442
> 
> 
> Repository: ambari
> 
> 
> Description
> ---
> 
> The StackAdvisor needs to set a config map for the `ams-grafana-env`, even it 
> is empty, to meet non-null expectations in Kerberos and Blueprint logic. 
> 
> To to this, the following line needs to be added to the StackAdvisor code:
> 
> #stacks/HDP/2.0.6/services/stack_advisor.py:473
> ```
> putGrafanaProperty = self.putProperty(configurations, "ams-grafana-env", 
> services)
> ```
> 
> 
> Diffs
> -
> 
>   
> ambari-server/src/main/java/org/apache/ambari/server/api/services/stackadvisor/StackAdvisorBlueprintProcessor.java
>  ea9fffb 
>   
> ambari-server/src/main/java/org/apache/ambari/server/controller/KerberosHelperImpl.java
>  be6edc9 
>   
> ambari-server/src/main/java/org/apache/ambari/server/controller/internal/BlueprintConfigurationProcessor.java
>  f5e7578 
>   ambari-server/src/main/resources/stacks/HDP/2.0.6/services/stack_advisor.py 
> 8c18421 
>   
> ambari-server/src/test/java/org/apache/ambari/server/api/services/stackadvisor/StackAdvisorBlueprintProcessorTest.java
>  c4e694b 
>   
> ambari-server/src/test/java/org/apache/ambari/server/controller/KerberosHelperTest.java
>  f6027f3 
>   
> ambari-server/src/test/java/org/apache/ambari/server/controller/internal/BlueprintConfigurationProcessorTest.java
>  68d5755 
>   ambari-server/src/test/python/stacks/2.2/common/test_stack_advisor.py 
> 9070c0a 
> 
> Diff: https://reviews.apache.org/r/44916/diff/
> 
> 
> Testing
> ---
> 
> Manually tested in cluster Kerbrerized using the Ambari UI.
> 
> 
> Thanks,
> 
> Robert Levas
> 
>

Re: Review Request 44916: StackAdvisor needs to set config map for ams-grafana-env to meet non-null expectations

2016-03-19 Thread Robert Levas 


> On March 17, 2016, 8:53 a.m., Jonathan Hurley wrote:
> > My only concern is for safety and clarify of code, does it makes sense to 
> > change the contract of the config to always return a non-null (if empty) 
> > map?

I attempted to go this route, but I got stuck where the Jackson JSON parser 
comes into play and fills the 
`org.apache.ambari.server.api.services.stackadvisor.recommendations.RecommendationResponse.BlueprintConfigurations`
 object.  This is where the null is getting set - or rather not set.  So if we 
wanted to persue this route, I can set a default value for the relevant class 
member and force it to never be null.


- Robert


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


On March 16, 2016, 4:55 p.m., Robert Levas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44916/
> ---
> 
> (Updated March 16, 2016, 4:55 p.m.)
> 
> 
> Review request for Ambari, Dmytro Sen, Jonathan Hurley, Nate Cole, and Sid 
> Wagle.
> 
> 
> Bugs: AMBARI-15442
> https://issues.apache.org/jira/browse/AMBARI-15442
> 
> 
> Repository: ambari
> 
> 
> Description
> ---
> 
> The StackAdvisor needs to set a config map for the `ams-grafana-env`, even it 
> is empty, to meet non-null expectations in Kerberos and Blueprint logic. 
> 
> To to this, the following line needs to be added to the StackAdvisor code:
> 
> #stacks/HDP/2.0.6/services/stack_advisor.py:473
> ```
> putGrafanaProperty = self.putProperty(configurations, "ams-grafana-env", 
> services)
> ```
> 
> 
> Diffs
> -
> 
>   
> ambari-server/src/main/java/org/apache/ambari/server/api/services/stackadvisor/StackAdvisorBlueprintProcessor.java
>  ea9fffb 
>   
> ambari-server/src/main/java/org/apache/ambari/server/controller/KerberosHelperImpl.java
>  be6edc9 
>   
> ambari-server/src/main/java/org/apache/ambari/server/controller/internal/BlueprintConfigurationProcessor.java
>  f5e7578 
>   ambari-server/src/main/resources/stacks/HDP/2.0.6/services/stack_advisor.py 
> 8c18421 
>   
> ambari-server/src/test/java/org/apache/ambari/server/api/services/stackadvisor/StackAdvisorBlueprintProcessorTest.java
>  c4e694b 
>   
> ambari-server/src/test/java/org/apache/ambari/server/controller/KerberosHelperTest.java
>  f6027f3 
>   
> ambari-server/src/test/java/org/apache/ambari/server/controller/internal/BlueprintConfigurationProcessorTest.java
>  68d5755 
>   ambari-server/src/test/python/stacks/2.2/common/test_stack_advisor.py 
> 9070c0a 
> 
> Diff: https://reviews.apache.org/r/44916/diff/
> 
> 
> Testing
> ---
> 
> Manually tested in cluster Kerbrerized using the Ambari UI.
> 
> 
> Thanks,
> 
> Robert Levas
> 
>

Re: Review Request 44916: StackAdvisor needs to set config map for ams-grafana-env to meet non-null expectations

2016-03-19 Thread Robert Levas 


> On March 16, 2016, 5:14 p.m., Jonathan Hurley wrote:
> > ambari-server/src/main/java/org/apache/ambari/server/api/services/stackadvisor/StackAdvisorBlueprintProcessor.java,
> >  line 224
> > 
> >
> > How can this ever be null? An empty config type!?

This is not config properties from a service config type, but updates generated 
by the stack advisor. Up until now, this hasn't been null, but it is possible 
based on the data the stack advisor returns.


- Robert


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


On March 16, 2016, 4:55 p.m., Robert Levas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44916/
> ---
> 
> (Updated March 16, 2016, 4:55 p.m.)
> 
> 
> Review request for Ambari, Dmytro Sen, Jonathan Hurley, Nate Cole, and Sid 
> Wagle.
> 
> 
> Bugs: AMBARI-15442
> https://issues.apache.org/jira/browse/AMBARI-15442
> 
> 
> Repository: ambari
> 
> 
> Description
> ---
> 
> The StackAdvisor needs to set a config map for the `ams-grafana-env`, even it 
> is empty, to meet non-null expectations in Kerberos and Blueprint logic. 
> 
> To to this, the following line needs to be added to the StackAdvisor code:
> 
> #stacks/HDP/2.0.6/services/stack_advisor.py:473
> ```
> putGrafanaProperty = self.putProperty(configurations, "ams-grafana-env", 
> services)
> ```
> 
> 
> Diffs
> -
> 
>   
> ambari-server/src/main/java/org/apache/ambari/server/api/services/stackadvisor/StackAdvisorBlueprintProcessor.java
>  ea9fffb 
>   
> ambari-server/src/main/java/org/apache/ambari/server/controller/KerberosHelperImpl.java
>  be6edc9 
>   
> ambari-server/src/main/java/org/apache/ambari/server/controller/internal/BlueprintConfigurationProcessor.java
>  f5e7578 
>   ambari-server/src/main/resources/stacks/HDP/2.0.6/services/stack_advisor.py 
> 8c18421 
>   
> ambari-server/src/test/java/org/apache/ambari/server/api/services/stackadvisor/StackAdvisorBlueprintProcessorTest.java
>  c4e694b 
>   
> ambari-server/src/test/java/org/apache/ambari/server/controller/KerberosHelperTest.java
>  f6027f3 
>   
> ambari-server/src/test/java/org/apache/ambari/server/controller/internal/BlueprintConfigurationProcessorTest.java
>  68d5755 
>   ambari-server/src/test/python/stacks/2.2/common/test_stack_advisor.py 
> 9070c0a 
> 
> Diff: https://reviews.apache.org/r/44916/diff/
> 
> 
> Testing
> ---
> 
> Manually tested in cluster Kerbrerized using the Ambari UI.
> 
> 
> Thanks,
> 
> Robert Levas
> 
>

Re: Review Request 44916: StackAdvisor needs to set config map for ams-grafana-env to meet non-null expectations

2016-03-19 Thread Robert Levas 


> On March 16, 2016, 1:57 p.m., Sid Wagle wrote:
> > Shouldn't we also have a non-null check in the backend code?

I am not sure.. I guess it depends on the expectations. Historically it appears 
that the property map is expected to not be null.  I am happy to add null 
protections in place I can find that need to be protected.


- Robert


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


On March 16, 2016, 12:59 p.m., Robert Levas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44916/
> ---
> 
> (Updated March 16, 2016, 12:59 p.m.)
> 
> 
> Review request for Ambari, Dmytro Sen, Jonathan Hurley, Nate Cole, and Sid 
> Wagle.
> 
> 
> Bugs: AMBARI-15442
> https://issues.apache.org/jira/browse/AMBARI-15442
> 
> 
> Repository: ambari
> 
> 
> Description
> ---
> 
> The StackAdvisor needs to set a config map for the `ams-grafana-env`, even it 
> is empty, to meet non-null expectations in Kerberos and Blueprint logic. 
> 
> To to this, the following line needs to be added to the StackAdvisor code:
> 
> #stacks/HDP/2.0.6/services/stack_advisor.py:473
> ```
> putGrafanaProperty = self.putProperty(configurations, "ams-grafana-env", 
> services)
> ```
> 
> 
> Diffs
> -
> 
>   ambari-server/src/main/resources/stacks/HDP/2.0.6/services/stack_advisor.py 
> 8c18421 
> 
> Diff: https://reviews.apache.org/r/44916/diff/
> 
> 
> Testing
> ---
> 
> Manually tested in cluster Kerbrerized using the Ambari UI.
> 
> 
> Thanks,
> 
> Robert Levas
> 
>

Re: Review Request 44916: StackAdvisor needs to set config map for ams-grafana-env to meet non-null expectations

2016-03-18 Thread Jonathan Hurley 

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




ambari-server/src/main/java/org/apache/ambari/server/api/services/stackadvisor/StackAdvisorBlueprintProcessor.java
 (line 224)


How can this ever be null? An empty config type!?


- Jonathan Hurley


On March 16, 2016, 4:55 p.m., Robert Levas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44916/
> ---
> 
> (Updated March 16, 2016, 4:55 p.m.)
> 
> 
> Review request for Ambari, Dmytro Sen, Jonathan Hurley, Nate Cole, and Sid 
> Wagle.
> 
> 
> Bugs: AMBARI-15442
> https://issues.apache.org/jira/browse/AMBARI-15442
> 
> 
> Repository: ambari
> 
> 
> Description
> ---
> 
> The StackAdvisor needs to set a config map for the `ams-grafana-env`, even it 
> is empty, to meet non-null expectations in Kerberos and Blueprint logic. 
> 
> To to this, the following line needs to be added to the StackAdvisor code:
> 
> #stacks/HDP/2.0.6/services/stack_advisor.py:473
> ```
> putGrafanaProperty = self.putProperty(configurations, "ams-grafana-env", 
> services)
> ```
> 
> 
> Diffs
> -
> 
>   
> ambari-server/src/main/java/org/apache/ambari/server/api/services/stackadvisor/StackAdvisorBlueprintProcessor.java
>  ea9fffb 
>   
> ambari-server/src/main/java/org/apache/ambari/server/controller/KerberosHelperImpl.java
>  be6edc9 
>   
> ambari-server/src/main/java/org/apache/ambari/server/controller/internal/BlueprintConfigurationProcessor.java
>  f5e7578 
>   ambari-server/src/main/resources/stacks/HDP/2.0.6/services/stack_advisor.py 
> 8c18421 
>   
> ambari-server/src/test/java/org/apache/ambari/server/api/services/stackadvisor/StackAdvisorBlueprintProcessorTest.java
>  c4e694b 
>   
> ambari-server/src/test/java/org/apache/ambari/server/controller/KerberosHelperTest.java
>  f6027f3 
>   
> ambari-server/src/test/java/org/apache/ambari/server/controller/internal/BlueprintConfigurationProcessorTest.java
>  68d5755 
>   ambari-server/src/test/python/stacks/2.2/common/test_stack_advisor.py 
> 9070c0a 
> 
> Diff: https://reviews.apache.org/r/44916/diff/
> 
> 
> Testing
> ---
> 
> Manually tested in cluster Kerbrerized using the Ambari UI.
> 
> 
> Thanks,
> 
> Robert Levas
> 
>