ambari-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "John Speidel" <jspei...@hortonworks.com>
Subject Re: Review Request 28472: role_command_order is not inherited from the parent stack
Date Thu, 04 Dec 2014 23:57:00 GMT

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


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message