> On May 17, 2016, 2:30 p.m., Nate Cole wrote:
> > ambari-server/src/main/java/org/apache/ambari/server/stack/StackModule.java,
> > lines 890-892
> > <https://reviews.apache.org/r/45169/diff/2/?file=1382543#file1382543line890>
> >
> > Really need to throw here or just log it and continue; ?
>
> Tim Thorpe wrote:
> When the StackManager encounters errors while reading the stack, it
> almost always ends up throwing an exception and ambari-server fails to start.
> I'm ok with just logging the issue for this and other errors but then you
> could think that everything is fine with the service specific upgrade xml
> unless you dig through the log.
>
> Nate Cole wrote:
> That's true, but in this case you aren't bound by reading the stack per
> se, you're acting on the applicability of an XML element. This is happening
> in mergeServiceCheckGrouping, implying you're only considering
> ServiceCheckGroupings anyway.
>
> If we actually used XSD (I have a future work-item for that) then sure,
> that could cause failure of stack. But in this case, I think a simple no-op
> continue; is fine.
>
> Tim Thorpe wrote:
> Jayush suggested that I needed to make sure when merging groups that all
> the groups were of the same type. So this applies to regular Grouping,
> ClusterGrouping, ServiceCheckGrouping, etc...
>
> List<Grouping> list = allGroupMap.get(group.name);
> if (list.size() > 0) {
> Grouping first = list.get(0);
> if (!first.getClass().equals(group.getClass())) {
> throw new AmbariException("Expected class: " +
> first.getClass() + " instead of " + group.getClass());
> }
> }
> allGroupMap.get(group.name).add(group);
>
> Basically with the above code, the logging would never occur when merging
> ServiceCheckGroupings. Do you think the above code should just log as well
> or is it more critical because it applies to groups other than
> ServiceCheckGroupings?
>
> Nate Cole wrote:
> Is that a bit restrictive? It means you can't add a ServiceCheckGrouping
> right after a "regular" Grouping? I might be missing the point.
Hi Nate, This is for merging the entries in a group. So if you want to add
FOO into SERVICE_CHECK_1, the type of the grouping in FOO's upgrade.xml should
match the group in the original upgrade.xml. This prevents you from having
inadventently making it a regular grouping (by omiting the
xsi:type="service-check"):
<group name="SERVICE_CHECK_1" title="All Service Checks">
<service name="FOO">
<component>BAR</component>
</service>
</group>
In the case above I properly formatted the xml for a regular grouping but I
could have misformatted it by using the service check syntax which would have
failed during XML parsing.
So basically the check is to make sure the xsi:type of the group match IF you
are adding priorities/services/stages from a service's upgrade xml. This code
is not applicable to adding new groups.
- Tim
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45169/#review133533
-----------------------------------------------------------
On May 18, 2016, 1:19 p.m., Tim Thorpe wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45169/
> -----------------------------------------------------------
>
> (Updated May 18, 2016, 1:19 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
> 5a18b3f
>
> 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/ClusterGrouping.java
> 3325469
>
> 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
> 1e040e6
> 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
> 6e27da6
> ambari-server/src/main/resources/stacks/HDP/2.5/upgrades/upgrade-2.5.xml
> d755516
>
> ambari-server/src/test/java/org/apache/ambari/server/stack/StackManagerMiscTest.java
> dda1e7a
>
> ambari-server/src/test/java/org/apache/ambari/server/state/stack/UpgradePackTest.java
> 15be8b4
>
> ambari-server/src/test/resources/stacks/HDP/2.0.5/services/HDFS/upgrades/HDP/2.2.0/upgrade_test_15388.xml
> PRE-CREATION
>
> ambari-server/src/test/resources/stacks/HDP/2.2.0/upgrades/upgrade_test_15388.xml
> PRE-CREATION
>
> ambari-server/src/test/resources/stacks_with_upgrade_cycle/HDP/2.2.0/metainfo.xml
> PRE-CREATION
>
> ambari-server/src/test/resources/stacks_with_upgrade_cycle/HDP/2.2.0/repos/hdp.json
> PRE-CREATION
>
> ambari-server/src/test/resources/stacks_with_upgrade_cycle/HDP/2.2.0/repos/repoinfo.xml
> PRE-CREATION
>
> ambari-server/src/test/resources/stacks_with_upgrade_cycle/HDP/2.2.0/repos/version-2.2.0.4-123.xml
> PRE-CREATION
>
> ambari-server/src/test/resources/stacks_with_upgrade_cycle/HDP/2.2.0/role_command_order.json
> PRE-CREATION
>
> ambari-server/src/test/resources/stacks_with_upgrade_cycle/HDP/2.2.0/services/HDFS/metainfo.xml
> PRE-CREATION
>
> ambari-server/src/test/resources/stacks_with_upgrade_cycle/HDP/2.2.0/services/HDFS/upgrades/HDP/2.2.0/upgrade_test_15388.xml
> PRE-CREATION
>
> ambari-server/src/test/resources/stacks_with_upgrade_cycle/HDP/2.2.0/upgrades/config-upgrade.xml
> PRE-CREATION
>
> ambari-server/src/test/resources/stacks_with_upgrade_cycle/HDP/2.2.0/upgrades/upgrade_test_15388.xml
> PRE-CREATION
>
> 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
>
>
