> On May 12, 2015, 3:03 p.m., Robert Nettleton wrote:
> > The change looks fine to me, although I think it would be better to have
> > more unit tests for this change.
> >
> > Could the submitter please detail the testing done for this patch, other
> > than "mvn clean test"? Once some functional testing is done on this (if it
> > hasn't been already), we can consider moving towards a merge.
> >
> > Thanks.
>
> John Speidel wrote:
> +1 on confirmation of functional testing.
>
> Emil Anca wrote:
> Thanks for the feedback. Will ask Costel to handle your concerns and have
> this updated.
Hi, I performed the following functional tests:
1) tested that the server returns an error when saving a blueprint with invalid
structure for the "configurations" element
2) tested that the server saves several correctly structured blueprints:
a) blueprint having an empty "configurations" element
b) blueprint having "configurations" with a single "configuration-type"
element
c) blueprint having "configurations" with multiple "configuration-type"
elements properly structured
I have also added new unit tests to account for success scenarions (a) and (b).
Thanks for the feedback, I hope this addresses your concerns.
- Costel
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33848/#review83402
-----------------------------------------------------------
On May 5, 2015, 3:27 p.m., Emil Anca wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33848/
> -----------------------------------------------------------
>
> (Updated May 5, 2015, 3:27 p.m.)
>
>
> Review request for Ambari, Costel Radulescu, John Speidel, Robert Levas, and
> Robert Nettleton.
>
>
> Bugs: AMBARI-10931
> https://issues.apache.org/jira/browse/AMBARI-10931
>
>
> Repository: ambari
>
>
> Description
> -------
>
> The Blueprints processor needs to have stronger validation during the process
> of handling a POST for a newly-submitted Blueprint from a REST client.
> The Blueprints structure requires that the "configurations" element be an
> array of Maps, with each Map representing a given configuration type
> ("core-site", "storm-site", etc).
> Typically, the configurations structure should look like:
> ""configurations": [
> {
> "mapred-env": {
> "properties":
> { "mapreduce_log_dir_prefix": "/grid/0/log/hadoop-mapreduce" }
> }
> },
> { "mapred-site": {
> "properties":
> { "install-test-mapred-site": "install-test-mapred-site-VALUE" }
> }
> },
> { "yarn-env": {
> "properties":
> { "yarn_log_dir_prefix": "/grid/0/log/hadoop" }
> }
> }
> ]"
> The snippet above is just an example to illustrate the expected format.
> Recent changes to the Blueprint's structure to handle "properties_attributes"
> (such as "final") have modified the structure of a Blueprint, so that
> configuration properties are now contained in a "properties" map, internal to
> each configuration type. Creating new Blueprints using this newer format (the
> older format is still acceptable to the processor) require that the
> configuration types are still enclosed in a map, even though an internal map
> is also present, which may be a source of confusion.
> An example of an incorrect format would be:
> ""configurations": [
> {
> "mapred-env": {
> "properties":
> { "mapreduce_log_dir_prefix": "/grid/0/log/hadoop-mapreduce" }
> },
> "mapred-site": {
> "properties":
> { "install-test-mapred-site": "install-test-mapred-site-VALUE" }
> },
> "yarn-env": {
> "properties":
> { "yarn_log_dir_prefix": "/grid/0/log/hadoop" }
> }
> ]
> "
> Please note that in the example above a single map is created, with all
> configuration elements being added to that entry's configuration type. This
> causes a corrupted Blueprint to be stored and used for deployments, since all
> configuration elements added in the original Blueprint will be registered
> under one of the config types used in the original Blueprint. This will cause
> many types of cluster startup failures, as the configuration elements will be
> added to incorrect locations.
> When the Blueprint processor receives a POST submission with a new Blueprint
> that includes this incorrect format for "configurations", the processor must
> reject this submission, and send back a relevant error code/message, so that
> the user can resolve this problem prior to attempting a cluster deployment.
> This issue will not affect the older format for Blueprints, and so any
> backwards-compatible Blueprints used should work fine. This issue will only
> occur when new Blueprints are submitted that use the newer syntax/format for
> defining configuration properties (with support for properties_attributes) as
> well.
> This JIRA is being created to track this issue, as it could potentially be a
> user experience issue going forward, and should probably be resolved in
> Ambari 2.1, if possible.
>
>
> Problem: Blueprint "configurations" element structure is not validated which
> leads to cluster creations error
> Solution: The "configuration" element is checked for the correct structure as
> a List of Maps containing a single configuration type
>
>
> Diffs
> -----
>
>
> ambari-server/src/main/java/org/apache/ambari/server/controller/internal/BlueprintResourceProvider.java
> aab5395
>
> ambari-server/src/test/java/org/apache/ambari/server/controller/internal/BlueprintResourceProviderTest.java
> 118a7be
>
> Diff: https://reviews.apache.org/r/33848/diff/
>
>
> Testing
> -------
>
> mvn clean test
>
>
> Thanks,
>
> Emil Anca
>
>
