----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28472/#review63920 -----------------------------------------------------------
ambari-server/src/main/java/org/apache/ambari/server/stack/StackDirectory.java <https://reviews.apache.org/r/28472/#comment106257> please use parens for one line 'if' blocks. Relying on indention is very error prone and is generally frowned upon in the community if (rcoFilePath != null) { ... } ambari-server/src/main/java/org/apache/ambari/server/stack/StackDirectory.java <https://reviews.apache.org/r/28472/#comment106258> Actually, you can get rid of this 'if' block and replace the condition on 405 with this condition 'if (rcoFilePath != null)' and move the file instantiation to the top of that block. ambari-server/src/main/java/org/apache/ambari/server/stack/StackDirectory.java <https://reviews.apache.org/r/28472/#comment106272> minor comment can be simplified to use logger params: LOG.info("Role command order info was loaded from file: {}", file.getAbsolutePath()); ambari-server/src/main/java/org/apache/ambari/server/stack/StackDirectory.java <https://reviews.apache.org/r/28472/#comment106273> might be good to provide rco file location in error msg ambari-server/src/main/java/org/apache/ambari/server/stack/StackDirectory.java <https://reviews.apache.org/r/28472/#comment106278> if an IOException is caught above, do we still want to create a roleCommandOrder with a null result? ambari-server/src/main/java/org/apache/ambari/server/stack/StackModule.java <https://reviews.apache.org/r/28472/#comment106279> sorry, I should have suggested this the first time but it would be cleaner to move this call to within mergeStackWithParent since that is where all of the stack merge logic is applied ambari-server/src/main/java/org/apache/ambari/server/state/StackInfo.java <https://reviews.apache.org/r/28472/#comment106282> Does roleCommandOrder need to be accounted for in equals() and hashCode()? I see that before your patch that some fields are not accounted for which seems wrong so I just wanted to confirm that all we care about for equality is name/varsion. ambari-server/src/main/java/org/apache/ambari/server/state/stack/StackRoleCommandOrder.java <https://reviews.apache.org/r/28472/#comment106283> This class should contain javadoc. ambari-server/src/main/java/org/apache/ambari/server/utils/StackRoleCommandOrderUtils.java <https://reviews.apache.org/r/28472/#comment106281> In my opinion it would make more sense to just have a merge method in StackRoleCommandOrder that takes a parent instead of having a separarate utility class for this. - John Speidel On Nov. 27, 2014, 1:32 p.m., Andrew Onischuk wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/28472/ > ----------------------------------------------------------- > > (Updated Nov. 27, 2014, 1:32 p.m.) > > > Review request for Ambari, Dmitro Lisnichenko and John Speidel. > > > Bugs: AMBARI-4504 > https://issues.apache.org/jira/browse/AMBARI-4504 > > > Repository: ambari > > > Description > ------- > > role_command_order is not inherited from the parent stack > > > Diffs > ----- > > > ambari-server/src/main/java/org/apache/ambari/server/metadata/RoleCommandOrder.java > 32668fa > > ambari-server/src/main/java/org/apache/ambari/server/stack/StackDirectory.java > 2a30e40 > ambari-server/src/main/java/org/apache/ambari/server/stack/StackModule.java > 20dba84 > ambari-server/src/main/java/org/apache/ambari/server/state/StackInfo.java > f19cf81 > > ambari-server/src/main/java/org/apache/ambari/server/state/stack/StackRoleCommandOrder.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/utils/StackRoleCommandOrderUtils.java > PRE-CREATION > ambari-server/src/main/resources/stacks/HDP/2.1/role_command_order.json > 2d152ba > ambari-server/src/main/resources/stacks/HDP/2.2/role_command_order.json > 72b49fa > > ambari-server/src/test/java/org/apache/ambari/server/stack/StackManagerTest.java > 0502e1a > ambari-server/src/test/resources/stacks/HDP/2.0.8/role_command_order.json > 1404ef6 > ambari-server/src/test/resources/stacks/HDP/2.1.1/role_command_order.json > 1404ef6 > > Diff: https://reviews.apache.org/r/28472/diff/ > > > Testing > ------- > > mvn clean test > > > Thanks, > > Andrew Onischuk > >
