You are viewing a plain text version of this content. The canonical link for it is here.

Posted to dev@ambari.apache.org by Sandor Magyari <sm...@hortonworks.com> on 2015/09/08 17:57:28 UTC

Review Request 38182: Blueprint: API call to create cluster instance with blueprint fails when blueprint_name is not correct

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

Review request for Ambari, John Speidel, Robert Nettleton, and Jeff Sposetti.

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

Repository: ambari

Description
-------

PROBLEM

When creating any resource like blueprint, cluster etc, you can specify the resource name both in the Url and Json payload as well. For example in case of a blueprint creation: api/v1/blueprints/sandboxURL Json: {"Blueprints":{"blueprint_name":"sandboxJson"}}. In this case the blueprint will be created as sandboxJson so the name specified in Json will override the name specifie din the Url. The behavoir is true for cluster creation and any other resource where you can specify name in Json as well. This could be a bit misleading so the API may return an error in case of resource names don't match.

SOLUTION

The solution would be to throw an error message (Error code 400) in case resource names are different. To achive this only for cluster creation would be quite complicated since resource handling is quite generic and in ClusterResourceProvider - which contains resource specific handling - only the request body is available, the url is not. The actual behaviour is that in PersistenceManagerImpl.create method the resource name (eg. blueprint_name) is inserted from the url in Json in case it's missing. So if resource name is specified in url than that will be used, if is specified both url and Json then latter will be used. Because of this behaviour I think we should hadnle this problem in PersistenceManagerImpl.create as you can see in attached patch. This will affect of course the creation of all resources.
All unittests were

Diffs
-----

ambari-server/src/main/java/org/apache/ambari/server/api/services/persistence/PersistenceManagerImpl.java 4db5611
ambari-server/src/test/java/org/apache/ambari/server/api/services/PersistenceManagerImplTest.java 9ff1506

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

Testing
-------

Manually tested blueprint and cluster creation, then cluster creation with blueprint. All unitests passed, it seems it doesn't affect normal functionality.

Thanks,

Sandor Magyari

Re: Review Request 38182: Blueprint: API call to create cluster instance with blueprint fails when blueprint_name is not correct

Posted by Sandor Magyari <sm...@hortonworks.com>.

(Updated Sept. 9, 2015, 3:01 p.m.)

Review request for Ambari, Robert Levas and Robert Nettleton.

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

Repository: ambari

Description
-------

PROBLEM

SOLUTION

Diffs
-----

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

Testing (updated)
-------

Manually tested blueprint and cluster creation, then cluster creation with blueprint. All unit tests passed, it seems it doesn't affect normal functionality.

Thanks,

Sandor Magyari

Re: Review Request 38182: Blueprint: API call to create cluster instance with blueprint fails when blueprint_name is not correct

Posted by Robert Levas <rl...@hortonworks.com>.


> On Sept. 8, 2015, 5:14 p.m., Robert Nettleton wrote:
> > ambari-server/src/main/java/org/apache/ambari/server/api/services/persistence/PersistenceManagerImpl.java, line 73
> > <https://reviews.apache.org/r/38182/diff/1/?file=1065274#file1065274line73>
> >
> >     I'm a little concerned with this modification, as it will affect basically every resource type managed by Ambari.  
> >     
> >     We probably should not make a change that could potentially affect API compatibility in the "v1" version of the API.  
> >     
> >     While it would be a less-generic solution, I think I would recommend building a solution that is specific to the Blueprints issue, to reduce the risk of creating an API incompatibility for all resource types managed by ambari-server. 
> >     
> >     John Speidel should probably comment on this further, since he was the lead on the API services in Ambari.

I agree with @rnettleton, however I do like the way this solution attempts to disambiguate a resource's name when this scenario occurs.  Wouldn't a more _local_ solution be to simply assume that the name in the URL overrides the name in the JSON document?  Or is it too late by the time Ambari knows that the resource being created it a Blueprint?


- Robert


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


On Sept. 9, 2015, 11:01 a.m., Sandor Magyari wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38182/
> -----------------------------------------------------------
> 
> (Updated Sept. 9, 2015, 11:01 a.m.)
> 
> 
> Review request for Ambari, Robert Levas and Robert Nettleton.
> 
> 
> Bugs: AMBARI-12989
>     https://issues.apache.org/jira/browse/AMBARI-12989
> 
> 
> Repository: ambari
> 
> 
> Description
> -------
> 
> PROBLEM
> 
> When creating any resource like blueprint, cluster etc, you can specify the resource name both in the Url and Json payload as well. For example in case of a blueprint creation: api/v1/blueprints/sandboxURL Json: {"Blueprints":{"blueprint_name":"sandboxJson"}}. In this case the blueprint will be created as sandboxJson so the name specified in Json will override the name specifie din the Url. The behavoir is true for cluster creation and any other resource where you can specify name in Json as well. This could be a bit misleading so the API may return an error in case of resource names don't match.
> 
> SOLUTION
> 
> The solution would be to throw an error message (Error code 400) in case resource names are different. To achive this only for cluster creation would be quite complicated since resource handling is quite generic and in ClusterResourceProvider - which contains resource specific handling - only the request body is available, the url is not. The actual behaviour is that in PersistenceManagerImpl.create method the resource name (eg. blueprint_name) is inserted from the url in Json in case it's missing. So if resource name is specified in url than that will be used, if is specified both url and Json then latter will be used. Because of this behaviour I think we should hadnle this problem in PersistenceManagerImpl.create as you can see in attached patch. This will affect of course the creation of all resources. 
> All unittests were
> 
> 
> Diffs
> -----
> 
>   ambari-server/src/main/java/org/apache/ambari/server/api/services/persistence/PersistenceManagerImpl.java 4db5611 
>   ambari-server/src/test/java/org/apache/ambari/server/api/services/PersistenceManagerImplTest.java 9ff1506 
> 
> Diff: https://reviews.apache.org/r/38182/diff/
> 
> 
> Testing
> -------
> 
> Manually tested blueprint and cluster creation, then cluster creation with blueprint. All unit tests passed, it seems it doesn't affect normal functionality.
> 
> 
> Thanks,
> 
> Sandor Magyari
> 
>

Re: Review Request 38182: Blueprint: API call to create cluster instance with blueprint fails when blueprint_name is not correct

Posted by Robert Nettleton <rn...@hortonworks.com>.

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


I think we should consider a different approach to fix this issue.  My concerns are listed in the issue below.  

Thanks.


ambari-server/src/main/java/org/apache/ambari/server/api/services/persistence/PersistenceManagerImpl.java (line 73)
<https://reviews.apache.org/r/38182/#comment154275>

    I'm a little concerned with this modification, as it will affect basically every resource type managed by Ambari.  
    
    We probably should not make a change that could potentially affect API compatibility in the "v1" version of the API.  
    
    While it would be a less-generic solution, I think I would recommend building a solution that is specific to the Blueprints issue, to reduce the risk of creating an API incompatibility for all resource types managed by ambari-server. 
    
    John Speidel should probably comment on this further, since he was the lead on the API services in Ambari.


- Robert Nettleton


On Sept. 8, 2015, 3:57 p.m., Sandor Magyari wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38182/
> -----------------------------------------------------------
> 
> (Updated Sept. 8, 2015, 3:57 p.m.)
> 
> 
> Review request for Ambari, John Speidel, Robert Nettleton, and Jeff Sposetti.
> 
> 
> Bugs: AMBARI-12989
>     https://issues.apache.org/jira/browse/AMBARI-12989
> 
> 
> Repository: ambari
> 
> 
> Description
> -------
> 
> PROBLEM
> 
> When creating any resource like blueprint, cluster etc, you can specify the resource name both in the Url and Json payload as well. For example in case of a blueprint creation: api/v1/blueprints/sandboxURL Json: {"Blueprints":{"blueprint_name":"sandboxJson"}}. In this case the blueprint will be created as sandboxJson so the name specified in Json will override the name specifie din the Url. The behavoir is true for cluster creation and any other resource where you can specify name in Json as well. This could be a bit misleading so the API may return an error in case of resource names don't match.
> 
> SOLUTION
> 
> The solution would be to throw an error message (Error code 400) in case resource names are different. To achive this only for cluster creation would be quite complicated since resource handling is quite generic and in ClusterResourceProvider - which contains resource specific handling - only the request body is available, the url is not. The actual behaviour is that in PersistenceManagerImpl.create method the resource name (eg. blueprint_name) is inserted from the url in Json in case it's missing. So if resource name is specified in url than that will be used, if is specified both url and Json then latter will be used. Because of this behaviour I think we should hadnle this problem in PersistenceManagerImpl.create as you can see in attached patch. This will affect of course the creation of all resources. 
> All unittests were
> 
> 
> Diffs
> -----
> 
>   ambari-server/src/main/java/org/apache/ambari/server/api/services/persistence/PersistenceManagerImpl.java 4db5611 
>   ambari-server/src/test/java/org/apache/ambari/server/api/services/PersistenceManagerImplTest.java 9ff1506 
> 
> Diff: https://reviews.apache.org/r/38182/diff/
> 
> 
> Testing
> -------
> 
> Manually tested blueprint and cluster creation, then cluster creation with blueprint. All unitests passed, it seems it doesn't affect normal functionality.
> 
> 
> Thanks,
> 
> Sandor Magyari
> 
>