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

Posted to dev@ambari.apache.org by Robert Nettleton <rn...@hortonworks.com> on 2015/05/26 19:58:39 UTC

Review Request 34671: Improves error handling for Blueprint POST requests that reference invalid Stack version numbers

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

Review request for Ambari, John Speidel, Mahadev Konar, Robert Levas, and Sumit Mohanty.

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

Repository: ambari

Description
-------

This patch addresses AMBARI-11393.

When a Blueprint was POST-ed with a reference to an invalid stack version number, the Blueprints processor would throw a "500 Server Error" back to the caller. This was due to the fact that this error case was not being properly handled by the processor. The "500 Server Error" did not include any useful information for debugging the failure.

In order to address this issue, this patch implements the following:

1. Modifies the BlueprintFactory.createStack() method to catch an "ObjectNotFoundException", rather than a "StackAccessException". Some code in Ambari, including the AmbariMetaInfo class used by the BlueprintFactory, throw "ObjectNotFoundException" or "ParentObjectNotFoundException" rather than "StackAccessException". Since "StackAccessException" and "ParentObjectNotFoundException" inherit from "ObjectNotFoundException", catching "ObjectNotFoundException" allows for the most flexibility in error handling this case.
2. Refactors the code that creates the Stack object to use an internal factory interface. The default implementation uses the Stack constructor as before. This approach allows for simpler unit testing, as tests can plug in mock factories to simulate various Stacks and Stack-related error conditions.
3. Adds a unit test to verify this change.

Diffs
-----

ambari-server/src/main/java/org/apache/ambari/server/topology/BlueprintFactory.java f02db81
ambari-server/src/test/java/org/apache/ambari/server/topology/BlueprintFactoryTest.java cd465cf

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

Testing
-------

1. Ran the ambari-server unit tests, which are all passing.
2. Attempted a POST attempt with a Blueprint that references an invalid stack version number ("HDP", version "99.99"), and verified that the correct error is thrown back to the client.
3. Ran a 3-node Blueprint cluster install with a valid Blueprint version, to make sure this change won't break deploying valid Blueprints.

Thanks,

Robert Nettleton

Re: Review Request 34671: Improves error handling for Blueprint POST requests that reference invalid Stack version numbers

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

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

Ship it!


Ship It!

- Robert Levas


On May 26, 2015, 2:47 p.m., Robert Nettleton wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34671/
> -----------------------------------------------------------
> 
> (Updated May 26, 2015, 2:47 p.m.)
> 
> 
> Review request for Ambari, John Speidel, Mahadev Konar, Robert Levas, and Sumit Mohanty.
> 
> 
> Bugs: AMBARI-11393
>     https://issues.apache.org/jira/browse/AMBARI-11393
> 
> 
> Repository: ambari
> 
> 
> Description
> -------
> 
> This patch addresses AMBARI-11393. 
> 
> When a Blueprint was POST-ed with a reference to an invalid stack version number, the Blueprints processor would throw a "500 Server Error" back to the caller. This was due to the fact that this error case was not being properly handled by the processor.  The "500 Server Error" did not include any useful information for debugging the failure.
> 
> In order to address this issue, this patch implements the following:
> 
> 1. Modifies the BlueprintFactory.createStack() method to catch an "ObjectNotFoundException", rather than a "StackAccessException".  Some code in Ambari, including the AmbariMetaInfo class used by the BlueprintFactory, throw "ObjectNotFoundException" or "ParentObjectNotFoundException" rather than "StackAccessException".  Since "StackAccessException" and "ParentObjectNotFoundException" inherit from "ObjectNotFoundException", catching "ObjectNotFoundException" allows for the most flexibility in error handling this case.  
> 2. Refactors the code that creates the Stack object to use an internal factory interface.  The default implementation uses the Stack constructor as before.  This approach allows for simpler unit testing, as tests can plug in mock factories to simulate various Stacks and Stack-related error conditions.  
> 3. Adds a unit test to verify this change.
> 
> 
> Diffs
> -----
> 
>   ambari-server/src/main/java/org/apache/ambari/server/topology/BlueprintFactory.java f02db81 
>   ambari-server/src/test/java/org/apache/ambari/server/topology/BlueprintFactoryTest.java cd465cf 
> 
> Diff: https://reviews.apache.org/r/34671/diff/
> 
> 
> Testing
> -------
> 
> 1. Ran the ambari-server unit tests, which are all passing.
> 2. Attempted a POST attempt with a Blueprint that references an invalid stack version number ("HDP", version "99.99"), and verified that the correct error is thrown back to the client. 
> 3. Ran a 3-node Blueprint cluster install with a valid Blueprint version, to make sure this change won't break deploying valid Blueprints.
> 
> 
> Thanks,
> 
> Robert Nettleton
> 
>

Re: Review Request 34671: Improves error handling for Blueprint POST requests that reference invalid Stack version numbers

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

(Updated May 26, 2015, 6:47 p.m.)

Review request for Ambari, John Speidel, Mahadev Konar, Robert Levas, and Sumit Mohanty.

Changes
-------

Uploaded Version 2 of the patch, which just removes the TODO mentioned by the reviewer.

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

Repository: ambari

Description
-------

This patch addresses AMBARI-11393.

In order to address this issue, this patch implements the following:

Diffs (updated)
-----

ambari-server/src/main/java/org/apache/ambari/server/topology/BlueprintFactory.java f02db81
ambari-server/src/test/java/org/apache/ambari/server/topology/BlueprintFactoryTest.java cd465cf

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

Testing
-------

Thanks,

Robert Nettleton

Re: Review Request 34671: Improves error handling for Blueprint POST requests that reference invalid Stack version numbers

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


> On May 26, 2015, 6:34 p.m., John Speidel wrote:
> > Looks good Bob, thanks.
> > One minor nit, would be good to remove the todo on line 110 now that you have introduced a stack factory.

Hi John, Thanks for the review!

Good catch on the TODO comment.  I'll remove it and re-upload the patch.


- Robert


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


On May 26, 2015, 5:58 p.m., Robert Nettleton wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34671/
> -----------------------------------------------------------
> 
> (Updated May 26, 2015, 5:58 p.m.)
> 
> 
> Review request for Ambari, John Speidel, Mahadev Konar, Robert Levas, and Sumit Mohanty.
> 
> 
> Bugs: AMBARI-11393
>     https://issues.apache.org/jira/browse/AMBARI-11393
> 
> 
> Repository: ambari
> 
> 
> Description
> -------
> 
> This patch addresses AMBARI-11393. 
> 
> When a Blueprint was POST-ed with a reference to an invalid stack version number, the Blueprints processor would throw a "500 Server Error" back to the caller. This was due to the fact that this error case was not being properly handled by the processor.  The "500 Server Error" did not include any useful information for debugging the failure.
> 
> In order to address this issue, this patch implements the following:
> 
> 1. Modifies the BlueprintFactory.createStack() method to catch an "ObjectNotFoundException", rather than a "StackAccessException".  Some code in Ambari, including the AmbariMetaInfo class used by the BlueprintFactory, throw "ObjectNotFoundException" or "ParentObjectNotFoundException" rather than "StackAccessException".  Since "StackAccessException" and "ParentObjectNotFoundException" inherit from "ObjectNotFoundException", catching "ObjectNotFoundException" allows for the most flexibility in error handling this case.  
> 2. Refactors the code that creates the Stack object to use an internal factory interface.  The default implementation uses the Stack constructor as before.  This approach allows for simpler unit testing, as tests can plug in mock factories to simulate various Stacks and Stack-related error conditions.  
> 3. Adds a unit test to verify this change.
> 
> 
> Diffs
> -----
> 
>   ambari-server/src/main/java/org/apache/ambari/server/topology/BlueprintFactory.java f02db81 
>   ambari-server/src/test/java/org/apache/ambari/server/topology/BlueprintFactoryTest.java cd465cf 
> 
> Diff: https://reviews.apache.org/r/34671/diff/
> 
> 
> Testing
> -------
> 
> 1. Ran the ambari-server unit tests, which are all passing.
> 2. Attempted a POST attempt with a Blueprint that references an invalid stack version number ("HDP", version "99.99"), and verified that the correct error is thrown back to the client. 
> 3. Ran a 3-node Blueprint cluster install with a valid Blueprint version, to make sure this change won't break deploying valid Blueprints.
> 
> 
> Thanks,
> 
> Robert Nettleton
> 
>

Re: Review Request 34671: Improves error handling for Blueprint POST requests that reference invalid Stack version numbers

Posted by John Speidel <js...@hortonworks.com>.

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

Ship it!


Looks good Bob, thanks.
One minor nit, would be good to remove the todo on line 110 now that you have introduced a stack factory.

- John Speidel


On May 26, 2015, 5:58 p.m., Robert Nettleton wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34671/
> -----------------------------------------------------------
> 
> (Updated May 26, 2015, 5:58 p.m.)
> 
> 
> Review request for Ambari, John Speidel, Mahadev Konar, Robert Levas, and Sumit Mohanty.
> 
> 
> Bugs: AMBARI-11393
>     https://issues.apache.org/jira/browse/AMBARI-11393
> 
> 
> Repository: ambari
> 
> 
> Description
> -------
> 
> This patch addresses AMBARI-11393. 
> 
> When a Blueprint was POST-ed with a reference to an invalid stack version number, the Blueprints processor would throw a "500 Server Error" back to the caller. This was due to the fact that this error case was not being properly handled by the processor.  The "500 Server Error" did not include any useful information for debugging the failure.
> 
> In order to address this issue, this patch implements the following:
> 
> 1. Modifies the BlueprintFactory.createStack() method to catch an "ObjectNotFoundException", rather than a "StackAccessException".  Some code in Ambari, including the AmbariMetaInfo class used by the BlueprintFactory, throw "ObjectNotFoundException" or "ParentObjectNotFoundException" rather than "StackAccessException".  Since "StackAccessException" and "ParentObjectNotFoundException" inherit from "ObjectNotFoundException", catching "ObjectNotFoundException" allows for the most flexibility in error handling this case.  
> 2. Refactors the code that creates the Stack object to use an internal factory interface.  The default implementation uses the Stack constructor as before.  This approach allows for simpler unit testing, as tests can plug in mock factories to simulate various Stacks and Stack-related error conditions.  
> 3. Adds a unit test to verify this change.
> 
> 
> Diffs
> -----
> 
>   ambari-server/src/main/java/org/apache/ambari/server/topology/BlueprintFactory.java f02db81 
>   ambari-server/src/test/java/org/apache/ambari/server/topology/BlueprintFactoryTest.java cd465cf 
> 
> Diff: https://reviews.apache.org/r/34671/diff/
> 
> 
> Testing
> -------
> 
> 1. Ran the ambari-server unit tests, which are all passing.
> 2. Attempted a POST attempt with a Blueprint that references an invalid stack version number ("HDP", version "99.99"), and verified that the correct error is thrown back to the client. 
> 3. Ran a 3-node Blueprint cluster install with a valid Blueprint version, to make sure this change won't break deploying valid Blueprints.
> 
> 
> Thanks,
> 
> Robert Nettleton
> 
>