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