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




ambari-server/src/main/java/org/apache/ambari/server/stack/StackModule.java 
(line 674)
<https://reviews.apache.org/r/45169/#comment198054>

    new ArrayList<>()



ambari-server/src/main/java/org/apache/ambari/server/stack/StackModule.java 
(line 705)
<https://reviews.apache.org/r/45169/#comment198057>

    Use constant instead
    See StackDefinitionDirectory::CONFIG_UPGRADE_XML_FILENAME_PREFIX



ambari-server/src/main/java/org/apache/ambari/server/stack/StackModule.java 
(line 734)
<https://reviews.apache.org/r/45169/#comment198058>

    new ArrayList<>()



ambari-server/src/main/java/org/apache/ambari/server/stack/StackModule.java 
(line 745)
<https://reviews.apache.org/r/45169/#comment198093>

    Validate that all sub-groups in the list are of the same type.



ambari-server/src/main/java/org/apache/ambari/server/stack/StackModule.java 
(line 753)
<https://reviews.apache.org/r/45169/#comment198066>

    Might not necessarily be original if this is a new group for the service. 
(Line#745).



ambari-server/src/main/java/org/apache/ambari/server/stack/StackModule.java 
(line 829)
<https://reviews.apache.org/r/45169/#comment198094>

    What about RestartGrouping, ColocatedGrouping, StartGrouping, StopGrouping, 
UpdateStackGrouping? How about parent.merge(iterator) instead?



ambari-server/src/main/java/org/apache/ambari/server/stack/StackModule.java 
(line 837)
<https://reviews.apache.org/r/45169/#comment198067>

    nit: child instead of next?



ambari-server/src/main/java/org/apache/ambari/server/stack/StackModule.java 
(line 844)
<https://reviews.apache.org/r/45169/#comment198092>

    The after tag is overloaded. Meaning it could mean insert this service 
after another service in some place and in some places it could mean insert 
this  group after another group. I am wondering if there would be a case where 
we would need a combination (example: Create a new group that is not in the 
stack upgrade pack and add 2 new services to this group in a specific order). 
    
    Instead of leaving the <after> tag to interpretation, why not be explicit 
as <add-group-after>, <add-services-after> etc. That way we can support 
combinations



ambari-server/src/main/resources/stacks/HDP/2.4/upgrades/upgrade-2.5.xml (line 
158)
<https://reviews.apache.org/r/45169/#comment198081>

    SERVICE_CHECK_1 instead


- Jayush Luniya


On May 16, 2016, 6:50 p.m., Tim Thorpe wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45169/
> -----------------------------------------------------------
> 
> (Updated May 16, 2016, 6:50 p.m.)
> 
> 
> Review request for Ambari, Alejandro Fernandez, Jonathan Hurley, Jayush 
> Luniya, and Nate Cole.
> 
> 
> Bugs: AMBARI-15388
>     https://issues.apache.org/jira/browse/AMBARI-15388
> 
> 
> Repository: ambari
> 
> 
> Description
> -------
> 
> Currently the upgrade is defined as a series of xml files specific to the 
> current stack version and the target stack version. Each upgrade xml defines 
> the overall sequence of the upgrade and what needs to be done for each 
> service. It would both easier to maintain and easier to add new services, if 
> the services themselves could specify what should be done during their 
> upgrade.
> 
> There are two ways to make these changes, the alternate approach would be to 
> only make the java changes and not split the upgrade xml files.  This would 
> still allow new services to add themselves into the upgrade.  The benefit of 
> this is that for the stack services you only have one upgrade xml file.  The 
> problem with that is it is easier for a particular service to have 
> unintentional changes between upgrade xml files.
> 
> 
> Diffs
> -----
> 
>   
> ambari-server/src/main/java/org/apache/ambari/server/stack/CommonServiceDirectory.java
>  7f7a49e 
>   
> ambari-server/src/main/java/org/apache/ambari/server/stack/ServiceDirectory.java
>  8a7b42b 
>   
> ambari-server/src/main/java/org/apache/ambari/server/stack/ServiceModule.java 
> f781574 
>   
> ambari-server/src/main/java/org/apache/ambari/server/stack/StackDirectory.java
>  13d5047 
>   ambari-server/src/main/java/org/apache/ambari/server/stack/StackModule.java 
> 6129ec0 
>   
> ambari-server/src/main/java/org/apache/ambari/server/stack/StackServiceDirectory.java
>  88f6e19 
>   ambari-server/src/main/java/org/apache/ambari/server/state/ServiceInfo.java 
> 43cefb9 
>   
> ambari-server/src/main/java/org/apache/ambari/server/state/stack/UpgradePack.java
>  b860731 
>   
> ambari-server/src/main/java/org/apache/ambari/server/state/stack/upgrade/Grouping.java
>  67d7fdb 
>   
> ambari-server/src/main/java/org/apache/ambari/server/state/stack/upgrade/ServiceCheckGrouping.java
>  5cda422 
>   ambari-server/src/main/resources/stacks/HDP/2.3/upgrades/upgrade-2.3.xml 
> 6b74af0 
>   ambari-server/src/main/resources/stacks/HDP/2.3/upgrades/upgrade-2.4.xml 
> 9fb2bba 
>   ambari-server/src/main/resources/stacks/HDP/2.3/upgrades/upgrade-2.5.xml 
> 1cd2ffa 
>   ambari-server/src/main/resources/stacks/HDP/2.4/upgrades/upgrade-2.4.xml 
> e3bc7a3 
>   ambari-server/src/main/resources/stacks/HDP/2.4/upgrades/upgrade-2.5.xml 
> 9c6a02d 
>   ambari-server/src/main/resources/stacks/HDP/2.5/upgrades/upgrade-2.5.xml 
> 1745de8 
> 
> Diff: https://reviews.apache.org/r/45169/diff/
> 
> 
> Testing
> -------
> 
> Manual testing so far.  I have the code read the upgrade xml and all of its 
> service specific xml files, built the upgrade pack and then write the full 
> upgrade xml to disk and then compare the results to the original upgrade xml.
> 
> This review is mostly for the design doc which is attached to the JIRA.  Not 
> sure how to create a review board with a design doc instead of a patch file.
> 
> 
> Thanks,
> 
> Tim Thorpe
> 
>

Reply via email to