----------------------------------------------------------- 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 > >
