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

Posted to reviews@ambari.apache.org by Attila Magyar <am...@hortonworks.com> on 2017/07/11 15:02:40 UTC

Review Request 60774: Ambari updates memory settings in blueprint incorrectly

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

Review request for Ambari, Laszlo Puskas, Robert Nettleton, Sandor Magyari, Sebastian Toader, and Sid Wagle.

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

Repository: ambari

Description
-------

Ambari appended a 'm' suffix to certain memory settings related properties during blueprint install. This was implemented in the MPropertyUpdater.

For example if namenode_heapsize was 512 then Ambari updated it to 512m. However if the property already had a suffix like 4g then it was updated to be 4gm.

This patch does 2 things differently
1. Instead of the hardcoded 'm' suffix, it uses the unit that is defined in the stack (if the stack doesn't define anything it falls back using 'm' as before).
2. It checks if the property already has some unit, and if that unit doesn't match the stack defined unit, then the blueprint will be rejected with an error (this case has never worked before)

For examples:
1. 4g is rejected if the stack defined unit is MB.
2. 4 becomes 4m if the stack defined unit is MB.
3. 4m stays 4m if the stack defined unit is MB.

MPropertyUpdated was replaced with UnitUpdater. But a new TopologyValidator was also introduced. The purpose of this is to catch invalid properties earlier.

Diffs
-----

ambari-server/src/main/java/org/apache/ambari/server/controller/internal/BlueprintConfigurationProcessor.java 37284be
ambari-server/src/main/java/org/apache/ambari/server/controller/internal/Stack.java e1ea1cd
ambari-server/src/main/java/org/apache/ambari/server/controller/internal/UnitUpdater.java PRE-CREATION
ambari-server/src/main/java/org/apache/ambari/server/topology/validators/TopologyValidatorFactory.java 5a6f64e
ambari-server/src/main/java/org/apache/ambari/server/topology/validators/UnitValidatedProperty.java PRE-CREATION
ambari-server/src/main/java/org/apache/ambari/server/topology/validators/UnitValidator.java PRE-CREATION
ambari-server/src/test/java/org/apache/ambari/server/controller/internal/UnitUpdaterTest.java PRE-CREATION
ambari-server/src/test/java/org/apache/ambari/server/topology/validators/UnitValidatorTest.java PRE-CREATION

Diff: https://reviews.apache.org/r/60774/diff/1/

Testing
-------

Manually with blueprints with the following values
- namenode_heapsize=512 -> transformed to 512m
- namenode_heapsize=512g -> exception
- namenode_heapsize=512m -> no transformation

existing tests: PENDING

Thanks,

Attila Magyar

Re: Review Request 60774: Ambari updates memory settings in blueprint incorrectly

Posted by Sebastian Toader <st...@hortonworks.com>.

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


Ship it!




Ship It!

- Sebastian Toader


On July 12, 2017, 2:51 p.m., Attila Magyar wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60774/
> -----------------------------------------------------------
> 
> (Updated July 12, 2017, 2:51 p.m.)
> 
> 
> Review request for Ambari, Laszlo Puskas, Robert Nettleton, Sandor Magyari, Sebastian Toader, and Sid Wagle.
> 
> 
> Bugs: AMBARI-21442
>     https://issues.apache.org/jira/browse/AMBARI-21442
> 
> 
> Repository: ambari
> 
> 
> Description
> -------
> 
> Ambari appended a 'm' suffix to certain memory settings related properties during blueprint install. This was implemented in the MPropertyUpdater.
> 
> For example if namenode_heapsize was 512 then Ambari updated it to 512m. However if the property already had a suffix like 4g then it was updated to be 4gm.
> 
> This patch does 2 things differently
>  1. Instead of the hardcoded 'm' suffix, it uses the unit that is defined in the stack (if the stack doesn't define anything it falls back using 'm' as before).
>  2. It checks if the property already has some unit, and if that unit doesn't match the stack defined unit, then the blueprint will be rejected with an error (this case has never worked before)
> 
> For examples:
>  1.   4g is rejected if the stack defined unit is MB.
>  2.   4 becomes 4m if the stack defined unit is MB.
>  3.   4m stays 4m if the stack defined unit is MB.
> 
> 
> MPropertyUpdated was replaced with UnitUpdater. But a new TopologyValidator was also introduced. The purpose of this is to catch invalid properties earlier.
> 
> 
> Diffs
> -----
> 
>   ambari-server/src/main/java/org/apache/ambari/server/controller/internal/BlueprintConfigurationProcessor.java 37284be 
>   ambari-server/src/main/java/org/apache/ambari/server/controller/internal/Stack.java e1ea1cd 
>   ambari-server/src/main/java/org/apache/ambari/server/controller/internal/UnitUpdater.java PRE-CREATION 
>   ambari-server/src/main/java/org/apache/ambari/server/topology/validators/TopologyValidatorFactory.java 5a6f64e 
>   ambari-server/src/main/java/org/apache/ambari/server/topology/validators/UnitValidatedProperty.java PRE-CREATION 
>   ambari-server/src/main/java/org/apache/ambari/server/topology/validators/UnitValidator.java PRE-CREATION 
>   ambari-server/src/test/java/org/apache/ambari/server/controller/internal/UnitUpdaterTest.java PRE-CREATION 
>   ambari-server/src/test/java/org/apache/ambari/server/topology/validators/UnitValidatorTest.java PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/60774/diff/5/
> 
> 
> Testing
> -------
> 
> Manually with blueprints with the following values
>  - namenode_heapsize=512  -> transformed to 512m
>  - namenode_heapsize=512g -> exception
>  - namenode_heapsize=512m -> no transformation
> 
> existing tests: PENDING
> 
> 
> Thanks,
> 
> Attila Magyar
> 
>

Re: Review Request 60774: Ambari updates memory settings in blueprint incorrectly

Posted by Attila Magyar <am...@hortonworks.com>.

(Updated July 12, 2017, 12:51 p.m.)

Review request for Ambari, Laszlo Puskas, Robert Nettleton, Sandor Magyari, Sebastian Toader, and Sid Wagle.

Changes
-------

Added HostGroup config validaiton.

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

Repository: ambari

Description
-------

Ambari appended a 'm' suffix to certain memory settings related properties during blueprint install. This was implemented in the MPropertyUpdater.

For example if namenode_heapsize was 512 then Ambari updated it to 512m. However if the property already had a suffix like 4g then it was updated to be 4gm.

For examples:
1. 4g is rejected if the stack defined unit is MB.
2. 4 becomes 4m if the stack defined unit is MB.
3. 4m stays 4m if the stack defined unit is MB.

MPropertyUpdated was replaced with UnitUpdater. But a new TopologyValidator was also introduced. The purpose of this is to catch invalid properties earlier.

Diffs (updated)
-----

Diff: https://reviews.apache.org/r/60774/diff/5/

Changes: https://reviews.apache.org/r/60774/diff/4-5/

Testing
-------

Manually with blueprints with the following values
- namenode_heapsize=512 -> transformed to 512m
- namenode_heapsize=512g -> exception
- namenode_heapsize=512m -> no transformation

existing tests: PENDING

Thanks,

Attila Magyar

Re: Review Request 60774: Ambari updates memory settings in blueprint incorrectly

Posted by Attila Magyar <am...@hortonworks.com>.

(Updated July 12, 2017, 12:38 p.m.)

Review request for Ambari, Laszlo Puskas, Robert Nettleton, Sandor Magyari, Sebastian Toader, and Sid Wagle.

Changes
-------

Using topology.getConfiguration() to get the config tu be validated. This also includes properties that are defined in the cluster creation template.

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

Repository: ambari

Description
-------

Ambari appended a 'm' suffix to certain memory settings related properties during blueprint install. This was implemented in the MPropertyUpdater.

For example if namenode_heapsize was 512 then Ambari updated it to 512m. However if the property already had a suffix like 4g then it was updated to be 4gm.

For examples:
1. 4g is rejected if the stack defined unit is MB.
2. 4 becomes 4m if the stack defined unit is MB.
3. 4m stays 4m if the stack defined unit is MB.

MPropertyUpdated was replaced with UnitUpdater. But a new TopologyValidator was also introduced. The purpose of this is to catch invalid properties earlier.

Diffs (updated)
-----

Diff: https://reviews.apache.org/r/60774/diff/4/

Changes: https://reviews.apache.org/r/60774/diff/3-4/

Testing
-------

Manually with blueprints with the following values
- namenode_heapsize=512 -> transformed to 512m
- namenode_heapsize=512g -> exception
- namenode_heapsize=512m -> no transformation

existing tests: PENDING

Thanks,

Attila Magyar

Re: Review Request 60774: Ambari updates memory settings in blueprint incorrectly

Posted by Attila Magyar <am...@hortonworks.com>.

(Updated July 12, 2017, 12:05 p.m.)

Review request for Ambari, Laszlo Puskas, Robert Nettleton, Sandor Magyari, Sebastian Toader, and Sid Wagle.

Changes
-------

Addressed review remarks

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

Repository: ambari

Description
-------

Ambari appended a 'm' suffix to certain memory settings related properties during blueprint install. This was implemented in the MPropertyUpdater.

For example if namenode_heapsize was 512 then Ambari updated it to 512m. However if the property already had a suffix like 4g then it was updated to be 4gm.

For examples:
1. 4g is rejected if the stack defined unit is MB.
2. 4 becomes 4m if the stack defined unit is MB.
3. 4m stays 4m if the stack defined unit is MB.

MPropertyUpdated was replaced with UnitUpdater. But a new TopologyValidator was also introduced. The purpose of this is to catch invalid properties earlier.

Diffs (updated)
-----

Diff: https://reviews.apache.org/r/60774/diff/3/

Changes: https://reviews.apache.org/r/60774/diff/2-3/

Testing
-------

Manually with blueprints with the following values
- namenode_heapsize=512 -> transformed to 512m
- namenode_heapsize=512g -> exception
- namenode_heapsize=512m -> no transformation

existing tests: PENDING

Thanks,

Attila Magyar

Re: Review Request 60774: Ambari updates memory settings in blueprint incorrectly

Posted by Sebastian Toader <st...@hortonworks.com>.

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




ambari-server/src/main/java/org/apache/ambari/server/controller/internal/BlueprintConfigurationProcessor.java
Lines 2777 (patched)
<https://reviews.apache.org/r/60774/#comment255368>

    Shouldn't this be rather ```Map<String, Set<UnitValidatedProperty>```. A config type shouldn't have duplicate properties by name



ambari-server/src/main/java/org/apache/ambari/server/topology/validators/UnitValidatedProperty.java
Lines 24 (patched)
<https://reviews.apache.org/r/60774/#comment255369>

    Implement hashcode and equals such as it can be used with sets or maps.
    
    The equalsverifier library can be used to easily create unit test that tests the hashcode and equals methods.



ambari-server/src/main/java/org/apache/ambari/server/topology/validators/UnitValidator.java
Lines 50 (patched)
<https://reviews.apache.org/r/60774/#comment255371>

    ```getProperties()``` I beleive returns only the config properties defined in the Blueprint but it doesn't the one defined in the cluster creation template. Check the ```getFullProperties()``` method.


- Sebastian Toader


On July 11, 2017, 5:02 p.m., Attila Magyar wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60774/
> -----------------------------------------------------------
> 
> (Updated July 11, 2017, 5:02 p.m.)
> 
> 
> Review request for Ambari, Laszlo Puskas, Robert Nettleton, Sandor Magyari, Sebastian Toader, and Sid Wagle.
> 
> 
> Bugs: AMBARI-21442
>     https://issues.apache.org/jira/browse/AMBARI-21442
> 
> 
> Repository: ambari
> 
> 
> Description
> -------
> 
> Ambari appended a 'm' suffix to certain memory settings related properties during blueprint install. This was implemented in the MPropertyUpdater.
> 
> For example if namenode_heapsize was 512 then Ambari updated it to 512m. However if the property already had a suffix like 4g then it was updated to be 4gm.
> 
> This patch does 2 things differently
>  1. Instead of the hardcoded 'm' suffix, it uses the unit that is defined in the stack (if the stack doesn't define anything it falls back using 'm' as before).
>  2. It checks if the property already has some unit, and if that unit doesn't match the stack defined unit, then the blueprint will be rejected with an error (this case has never worked before)
> 
> For examples:
>  1.   4g is rejected if the stack defined unit is MB.
>  2.   4 becomes 4m if the stack defined unit is MB.
>  3.   4m stays 4m if the stack defined unit is MB.
> 
> 
> MPropertyUpdated was replaced with UnitUpdater. But a new TopologyValidator was also introduced. The purpose of this is to catch invalid properties earlier.
> 
> 
> Diffs
> -----
> 
>   ambari-server/src/main/java/org/apache/ambari/server/controller/internal/BlueprintConfigurationProcessor.java 37284be 
>   ambari-server/src/main/java/org/apache/ambari/server/controller/internal/Stack.java e1ea1cd 
>   ambari-server/src/main/java/org/apache/ambari/server/controller/internal/UnitUpdater.java PRE-CREATION 
>   ambari-server/src/main/java/org/apache/ambari/server/topology/validators/TopologyValidatorFactory.java 5a6f64e 
>   ambari-server/src/main/java/org/apache/ambari/server/topology/validators/UnitValidatedProperty.java PRE-CREATION 
>   ambari-server/src/main/java/org/apache/ambari/server/topology/validators/UnitValidator.java PRE-CREATION 
>   ambari-server/src/test/java/org/apache/ambari/server/controller/internal/UnitUpdaterTest.java PRE-CREATION 
>   ambari-server/src/test/java/org/apache/ambari/server/topology/validators/UnitValidatorTest.java PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/60774/diff/2/
> 
> 
> Testing
> -------
> 
> Manually with blueprints with the following values
>  - namenode_heapsize=512  -> transformed to 512m
>  - namenode_heapsize=512g -> exception
>  - namenode_heapsize=512m -> no transformation
> 
> existing tests: PENDING
> 
> 
> Thanks,
> 
> Attila Magyar
> 
>