You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@ambari.apache.org by Oliver Szabo <os...@hortonworks.com> on 2015/09/23 20:40:00 UTC
Review Request 38673: Improve error checking for blueprint resource
creation
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38673/
-----------------------------------------------------------
Review request for Ambari, Robert Levas and Robert Nettleton.
Bugs: AMBARI-13202
https://issues.apache.org/jira/browse/AMBARI-13202
Repository: ambari
Description
-------
Improve error checking for blueprint resource creation.
----
To avoid NPE during blueprint resource creation (when request body is missing)
Question:
- if this validation is in better place at resource service level, we could include bean-validation libary and use @NotNull (or something similar) on specific POST methods where needed ... or a maybe private method enough for BlueprintService to check the body (as like: ClusterService#hasPermission)
Diffs
-----
ambari-server/src/main/java/org/apache/ambari/server/controller/internal/BlueprintResourceProvider.java f85ec32
ambari-server/src/test/java/org/apache/ambari/server/controller/internal/BlueprintResourceProviderTest.java 4a5ff46
Diff: https://reviews.apache.org/r/38673/diff/
Testing
-------
Unit testing done.
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time: 01:24 h
[INFO] Finished at: 2015-09-23T16:47:06+00:00
[INFO] Final Memory: 48M/589M
[INFO] ------------------------------------------------------------------------
Functional tessting: manually, POST blueprint resource without body
Thanks,
Oliver Szabo
Re: Review Request 38673: Improve error checking for blueprint
resource creation
Posted by Robert Nettleton <rn...@hortonworks.com>.
> On Sept. 24, 2015, 4:17 p.m., Robert Nettleton wrote:
> > Ship It!
I've merged this patch into trunk and branch-2.1.
Oliver, please close out this review as "Submitted" when you get a chance.
Thanks.
Test results for ambari-server:
python tests:
Total run:765
Total errors:0
Total failures:0
Java tests:
Results :
Tests run: 3203, Failures: 0, Errors: 0, Skipped: 25
- Robert
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38673/#review100430
-----------------------------------------------------------
On Sept. 24, 2015, 3:51 p.m., Oliver Szabo wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38673/
> -----------------------------------------------------------
>
> (Updated Sept. 24, 2015, 3:51 p.m.)
>
>
> Review request for Ambari, Robert Levas and Robert Nettleton.
>
>
> Bugs: AMBARI-13202
> https://issues.apache.org/jira/browse/AMBARI-13202
>
>
> Repository: ambari
>
>
> Description
> -------
>
> Improve error checking for blueprint resource creation.
> ----
> To avoid NPE during blueprint resource creation (when request body is missing)
>
> Question:
> - if this validation is in better place at resource service level, we could include bean-validation libary and use @NotNull (or something similar) on specific POST methods where needed ... or a maybe private method enough for BlueprintService to check the body (as like: ClusterService#hasPermission)
>
>
> Diffs
> -----
>
> ambari-server/src/main/java/org/apache/ambari/server/controller/internal/BlueprintResourceProvider.java 5fa6655
> ambari-server/src/test/java/org/apache/ambari/server/controller/internal/BlueprintResourceProviderTest.java 4a5ff46
>
> Diff: https://reviews.apache.org/r/38673/diff/
>
>
> Testing
> -------
>
> Unit testing done.
> [INFO] ------------------------------------------------------------------------
> [INFO] BUILD SUCCESS
> [INFO] ------------------------------------------------------------------------
> [INFO] Total time: 01:24 h
> [INFO] Finished at: 2015-09-23T16:47:06+00:00
> [INFO] Final Memory: 48M/589M
> [INFO] ------------------------------------------------------------------------
>
> Functional tessting: manually, POST blueprint resource without body
>
>
> Thanks,
>
> Oliver Szabo
>
>
Re: Review Request 38673: Improve error checking for blueprint
resource creation
Posted by Robert Nettleton <rn...@hortonworks.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38673/#review100430
-----------------------------------------------------------
Ship it!
Ship It!
- Robert Nettleton
On Sept. 24, 2015, 3:51 p.m., Oliver Szabo wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38673/
> -----------------------------------------------------------
>
> (Updated Sept. 24, 2015, 3:51 p.m.)
>
>
> Review request for Ambari, Robert Levas and Robert Nettleton.
>
>
> Bugs: AMBARI-13202
> https://issues.apache.org/jira/browse/AMBARI-13202
>
>
> Repository: ambari
>
>
> Description
> -------
>
> Improve error checking for blueprint resource creation.
> ----
> To avoid NPE during blueprint resource creation (when request body is missing)
>
> Question:
> - if this validation is in better place at resource service level, we could include bean-validation libary and use @NotNull (or something similar) on specific POST methods where needed ... or a maybe private method enough for BlueprintService to check the body (as like: ClusterService#hasPermission)
>
>
> Diffs
> -----
>
> ambari-server/src/main/java/org/apache/ambari/server/controller/internal/BlueprintResourceProvider.java 5fa6655
> ambari-server/src/test/java/org/apache/ambari/server/controller/internal/BlueprintResourceProviderTest.java 4a5ff46
>
> Diff: https://reviews.apache.org/r/38673/diff/
>
>
> Testing
> -------
>
> Unit testing done.
> [INFO] ------------------------------------------------------------------------
> [INFO] BUILD SUCCESS
> [INFO] ------------------------------------------------------------------------
> [INFO] Total time: 01:24 h
> [INFO] Finished at: 2015-09-23T16:47:06+00:00
> [INFO] Final Memory: 48M/589M
> [INFO] ------------------------------------------------------------------------
>
> Functional tessting: manually, POST blueprint resource without body
>
>
> Thanks,
>
> Oliver Szabo
>
>
Re: Review Request 38673: Improve error checking for blueprint
resource creation
Posted by Sumit Mohanty <sm...@hortonworks.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38673/#review100432
-----------------------------------------------------------
Ship it!
Ship It!
- Sumit Mohanty
On Sept. 24, 2015, 3:51 p.m., Oliver Szabo wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38673/
> -----------------------------------------------------------
>
> (Updated Sept. 24, 2015, 3:51 p.m.)
>
>
> Review request for Ambari, Robert Levas and Robert Nettleton.
>
>
> Bugs: AMBARI-13202
> https://issues.apache.org/jira/browse/AMBARI-13202
>
>
> Repository: ambari
>
>
> Description
> -------
>
> Improve error checking for blueprint resource creation.
> ----
> To avoid NPE during blueprint resource creation (when request body is missing)
>
> Question:
> - if this validation is in better place at resource service level, we could include bean-validation libary and use @NotNull (or something similar) on specific POST methods where needed ... or a maybe private method enough for BlueprintService to check the body (as like: ClusterService#hasPermission)
>
>
> Diffs
> -----
>
> ambari-server/src/main/java/org/apache/ambari/server/controller/internal/BlueprintResourceProvider.java 5fa6655
> ambari-server/src/test/java/org/apache/ambari/server/controller/internal/BlueprintResourceProviderTest.java 4a5ff46
>
> Diff: https://reviews.apache.org/r/38673/diff/
>
>
> Testing
> -------
>
> Unit testing done.
> [INFO] ------------------------------------------------------------------------
> [INFO] BUILD SUCCESS
> [INFO] ------------------------------------------------------------------------
> [INFO] Total time: 01:24 h
> [INFO] Finished at: 2015-09-23T16:47:06+00:00
> [INFO] Final Memory: 48M/589M
> [INFO] ------------------------------------------------------------------------
>
> Functional tessting: manually, POST blueprint resource without body
>
>
> Thanks,
>
> Oliver Szabo
>
>
Re: Review Request 38673: Improve error checking for blueprint
resource creation
Posted by Oliver Szabo <os...@hortonworks.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38673/
-----------------------------------------------------------
(Updated Sept. 24, 2015, 3:51 p.m.)
Review request for Ambari, Robert Levas and Robert Nettleton.
Bugs: AMBARI-13202
https://issues.apache.org/jira/browse/AMBARI-13202
Repository: ambari
Description
-------
Improve error checking for blueprint resource creation.
----
To avoid NPE during blueprint resource creation (when request body is missing)
Question:
- if this validation is in better place at resource service level, we could include bean-validation libary and use @NotNull (or something similar) on specific POST methods where needed ... or a maybe private method enough for BlueprintService to check the body (as like: ClusterService#hasPermission)
Diffs (updated)
-----
ambari-server/src/main/java/org/apache/ambari/server/controller/internal/BlueprintResourceProvider.java 5fa6655
ambari-server/src/test/java/org/apache/ambari/server/controller/internal/BlueprintResourceProviderTest.java 4a5ff46
Diff: https://reviews.apache.org/r/38673/diff/
Testing
-------
Unit testing done.
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time: 01:24 h
[INFO] Finished at: 2015-09-23T16:47:06+00:00
[INFO] Final Memory: 48M/589M
[INFO] ------------------------------------------------------------------------
Functional tessting: manually, POST blueprint resource without body
Thanks,
Oliver Szabo
Re: Review Request 38673: Improve error checking for blueprint
resource creation
Posted by Oliver Szabo <os...@hortonworks.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38673/
-----------------------------------------------------------
(Updated Sept. 24, 2015, 3:23 p.m.)
Review request for Ambari, Robert Levas and Robert Nettleton.
Changes
-------
refactored message & added some tests
Bugs: AMBARI-13202
https://issues.apache.org/jira/browse/AMBARI-13202
Repository: ambari
Description
-------
Improve error checking for blueprint resource creation.
----
To avoid NPE during blueprint resource creation (when request body is missing)
Question:
- if this validation is in better place at resource service level, we could include bean-validation libary and use @NotNull (or something similar) on specific POST methods where needed ... or a maybe private method enough for BlueprintService to check the body (as like: ClusterService#hasPermission)
Diffs (updated)
-----
ambari-server/src/main/java/org/apache/ambari/server/controller/internal/BlueprintResourceProvider.java 5fa6655
ambari-server/src/test/java/org/apache/ambari/server/controller/internal/BlueprintResourceProviderTest.java 4a5ff46
Diff: https://reviews.apache.org/r/38673/diff/
Testing
-------
Unit testing done.
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time: 01:24 h
[INFO] Finished at: 2015-09-23T16:47:06+00:00
[INFO] Final Memory: 48M/589M
[INFO] ------------------------------------------------------------------------
Functional tessting: manually, POST blueprint resource without body
Thanks,
Oliver Szabo
Re: Review Request 38673: Improve error checking for blueprint
resource creation
Posted by Robert Nettleton <rn...@hortonworks.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38673/#review100279
-----------------------------------------------------------
Ship it!
ambari-server/src/main/java/org/apache/ambari/server/controller/internal/BlueprintResourceProvider.java (line 384)
<https://reviews.apache.org/r/38673/#comment157429>
I would recommend changing this to something like:
"Request body for Blueprint create request is empty".
the "raw" mention is probably just confusing for users, and so I'd make the error message a little more straightforward.
Thanks.
ambari-server/src/main/java/org/apache/ambari/server/controller/internal/BlueprintResourceProvider.java (line 390)
<https://reviews.apache.org/r/38673/#comment157434>
I like this use of the Preconditions class, as it really improves the readability of this error-checking code.
Could you just verify that these two error checks are verified in the unit tests? If not, could you add some assertions to verify this change?
Thanks.
ambari-server/src/test/java/org/apache/ambari/server/controller/internal/BlueprintResourceProviderTest.java (line 173)
<https://reviews.apache.org/r/38673/#comment157432>
Minor issue:
It might be a good idea to explicitly configure the mock here to return a null request body.
While I realize that the mock will return null by default, I believe that the explicit set will make the intent of the test more obvious, and thus easier to maintain in the future.
The patch looks fine to me overall, with a few minor issues that should be addressed.
Thanks!
- Robert Nettleton
On Sept. 23, 2015, 6:39 p.m., Oliver Szabo wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38673/
> -----------------------------------------------------------
>
> (Updated Sept. 23, 2015, 6:39 p.m.)
>
>
> Review request for Ambari, Robert Levas and Robert Nettleton.
>
>
> Bugs: AMBARI-13202
> https://issues.apache.org/jira/browse/AMBARI-13202
>
>
> Repository: ambari
>
>
> Description
> -------
>
> Improve error checking for blueprint resource creation.
> ----
> To avoid NPE during blueprint resource creation (when request body is missing)
>
> Question:
> - if this validation is in better place at resource service level, we could include bean-validation libary and use @NotNull (or something similar) on specific POST methods where needed ... or a maybe private method enough for BlueprintService to check the body (as like: ClusterService#hasPermission)
>
>
> Diffs
> -----
>
> ambari-server/src/main/java/org/apache/ambari/server/controller/internal/BlueprintResourceProvider.java f85ec32
> ambari-server/src/test/java/org/apache/ambari/server/controller/internal/BlueprintResourceProviderTest.java 4a5ff46
>
> Diff: https://reviews.apache.org/r/38673/diff/
>
>
> Testing
> -------
>
> Unit testing done.
> [INFO] ------------------------------------------------------------------------
> [INFO] BUILD SUCCESS
> [INFO] ------------------------------------------------------------------------
> [INFO] Total time: 01:24 h
> [INFO] Finished at: 2015-09-23T16:47:06+00:00
> [INFO] Final Memory: 48M/589M
> [INFO] ------------------------------------------------------------------------
>
> Functional tessting: manually, POST blueprint resource without body
>
>
> Thanks,
>
> Oliver Szabo
>
>