You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@ambari.apache.org by Andrew Onischuk <ao...@hortonworks.com> on 2014/11/26 15:50:32 UTC
Review Request 28472: role_command_order is not inherited from the
parent stack
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28472/
-----------------------------------------------------------
Review request for Ambari and Dmitro Lisnichenko.
Bugs: AMBARI-4504
https://issues.apache.org/jira/browse/AMBARI-4504
Repository: ambari
Description
-------
Diffs
-----
ambari-server/src/main/java/org/apache/ambari/server/metadata/RoleCommandOrder.java 32668fa
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/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
Re: Review Request 28472: role_command_order is not inherited from the
parent stack
Posted by Dmitro Lisnichenko <dl...@hortonworks.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28472/#review64226
-----------------------------------------------------------
Ship it!
Ship It!
- Dmitro Lisnichenko
On Dec. 5, 2014, 5:55 p.m., Andrew Onischuk wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28472/
> -----------------------------------------------------------
>
> (Updated Dec. 5, 2014, 5:55 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 3629b36
> 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/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 940c65a
> 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
>
>
Re: Review Request 28472: role_command_order is not inherited from the
parent stack
Posted by Andrew Onischuk <ao...@hortonworks.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28472/
-----------------------------------------------------------
(Updated Dec. 8, 2014, 4:09 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 (updated)
-----
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 3629b36
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/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 940c65a
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
Re: Review Request 28472: role_command_order is not inherited from the
parent stack
Posted by John Speidel <js...@hortonworks.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28472/#review64072
-----------------------------------------------------------
Ship it!
The patch looks good, thanks for taking the time to incorporate the requested changes.
Please fix the 2 new issues and then feel free to merge the patch.
ambari-server/src/main/java/org/apache/ambari/server/stack/StackDirectory.java
<https://reviews.apache.org/r/28472/#comment106497>
There is no matching Logger method signature for error(String format, Object arg, Throwable t). Instead it is matching error(String format, Object... arguments) which you don't want because you only have one arg and an exception. The method signature that takes an exception is error(String msg, Throwable t) so you could do something like the following: LOG.error(String.format("Can not read role command order info %s", rcoFilePath), e)
ambari-server/src/main/java/org/apache/ambari/server/state/stack/StackRoleCommandOrder.java
<https://reviews.apache.org/r/28472/#comment106492>
Just noticed that you are using a deprecated class. Please replace MultiHashMap with the improved MultiValueMap.
- John Speidel
On Dec. 5, 2014, 5:55 p.m., Andrew Onischuk wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28472/
> -----------------------------------------------------------
>
> (Updated Dec. 5, 2014, 5:55 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 3629b36
> 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/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 940c65a
> 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
>
>
Re: Review Request 28472: role_command_order is not inherited from the
parent stack
Posted by Andrew Onischuk <ao...@hortonworks.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28472/
-----------------------------------------------------------
(Updated Dec. 5, 2014, 5:55 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 (updated)
-----
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 3629b36
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/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 940c65a
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
Re: Review Request 28472: role_command_order is not inherited from the
parent stack
Posted by Dmitro Lisnichenko <dl...@hortonworks.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28472/#review63353
-----------------------------------------------------------
Ship it!
Ship It!
- Dmitro Lisnichenko
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
>
>
Re: Review Request 28472: role_command_order is not inherited from the
parent stack
Posted by John Speidel <js...@hortonworks.com>.
-----------------------------------------------------------
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
>
>
Re: Review Request 28472: role_command_order is not inherited from the
parent stack
Posted by Andrew Onischuk <ao...@hortonworks.com>.
-----------------------------------------------------------
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 (updated)
-----
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
Re: Review Request 28472: role_command_order is not inherited from the
parent stack
Posted by Andrew Onischuk <ao...@hortonworks.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28472/
-----------------------------------------------------------
(Updated Nov. 26, 2014, 7:50 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 (updated)
-------
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/StackModule.java 20dba84
ambari-server/src/main/java/org/apache/ambari/server/state/StackInfo.java f19cf81
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
Re: Review Request 28472: role_command_order is not inherited from the
parent stack
Posted by John Speidel <js...@hortonworks.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28472/#review63110
-----------------------------------------------------------
ambari-server/src/main/java/org/apache/ambari/server/stack/StackModule.java
<https://reviews.apache.org/r/28472/#comment105300>
No references to IO should be in this class
ambari-server/src/main/java/org/apache/ambari/server/stack/StackModule.java
<https://reviews.apache.org/r/28472/#comment105299>
Belongs in StackDirectory
ambari-server/src/main/java/org/apache/ambari/server/stack/StackModule.java
<https://reviews.apache.org/r/28472/#comment105296>
This logic should occur prior to mergeServicesWithExplicitParent() invocaiton. Get rid of this if block and use the existing one above.
ambari-server/src/main/java/org/apache/ambari/server/stack/StackModule.java
<https://reviews.apache.org/r/28472/#comment105293>
StackModule shouldn't do any IO operations. IO is abstracted via StackDirectory.
ambari-server/src/main/java/org/apache/ambari/server/stack/StackModule.java
<https://reviews.apache.org/r/28472/#comment105294>
The RCO file location is available from StackDirectory. Should update StackDirectory to to all processing of this file.
ambari-server/src/main/java/org/apache/ambari/server/stack/StackModule.java
<https://reviews.apache.org/r/28472/#comment105302>
Since the logic is fairly substantial to merge RCO data, it should probably represented in a separate class, something lile StackRoleCommandOrder. StackDirectory would parse the file into an instance of this class and StackModule would get it from StackDirectory. To merge you could just call a merge method, perhaps something like 'StackRoleCommandOrder merge(StackRoleCommandOrder rco)'.
ambari-server/src/main/java/org/apache/ambari/server/stack/StackModule.java
<https://reviews.apache.org/r/28472/#comment105297>
All IO belongs in StackDirectory.
ambari-server/src/main/java/org/apache/ambari/server/stack/StackModule.java
<https://reviews.apache.org/r/28472/#comment105298>
This method belongs in StackDirectory.
Introduce a parse and get method in StackDirectory for RCO data.
- John Speidel
On Nov. 26, 2014, 2:50 p.m., Andrew Onischuk wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28472/
> -----------------------------------------------------------
>
> (Updated Nov. 26, 2014, 2:50 p.m.)
>
>
> Review request for Ambari and Dmitro Lisnichenko.
>
>
> Bugs: AMBARI-4504
> https://issues.apache.org/jira/browse/AMBARI-4504
>
>
> Repository: ambari
>
>
> Description
> -------
>
>
> Diffs
> -----
>
> ambari-server/src/main/java/org/apache/ambari/server/metadata/RoleCommandOrder.java 32668fa
> 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/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
>
>