----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/45169/#review133533 -----------------------------------------------------------
ambari-server/src/main/java/org/apache/ambari/server/stack/StackModule.java (line 743) <https://reviews.apache.org/r/45169/#comment198009> nit: can use newer collections syntax without the generic specifics: List<Grouping> list = new ArrayList<>(); ambari-server/src/main/java/org/apache/ambari/server/stack/StackModule.java (line 750) <https://reviews.apache.org/r/45169/#comment198008> nit: can use newer collections syntax without the generic specifics: Map<String, Grouping> map = new HashMap<>(); ambari-server/src/main/java/org/apache/ambari/server/stack/StackModule.java (line 764) <https://reviews.apache.org/r/45169/#comment198011> could use a MultiMap? ambari-server/src/main/java/org/apache/ambari/server/stack/StackModule.java (lines 765 - 766) <https://reviews.apache.org/r/45169/#comment198010> An Entry<String, Grouping> iterator is preferred ambari-server/src/main/java/org/apache/ambari/server/stack/StackModule.java (lines 822 - 831) <https://reviews.apache.org/r/45169/#comment198012> Visitor or abstract method? ambari-server/src/main/java/org/apache/ambari/server/stack/StackModule.java (lines 836 - 837) <https://reviews.apache.org/r/45169/#comment198017> Need an iterator directly? ambari-server/src/main/java/org/apache/ambari/server/stack/StackModule.java (lines 890 - 892) <https://reviews.apache.org/r/45169/#comment198022> Really need to throw here or just log it and continue; ? ambari-server/src/main/java/org/apache/ambari/server/stack/StackModule.java (line 934) <https://reviews.apache.org/r/45169/#comment198005> Some javadoc would be nice ambari-server/src/main/java/org/apache/ambari/server/stack/StackServiceDirectory.java (lines 112 - 113) <https://reviews.apache.org/r/45169/#comment198001> With no other exceptions, can just use "regular" logging syntax: LOG.debug("Service upgrades folder {} for service {} in stack {} is empty.", absUpgradesDir, serviceDir.getName(), stackId) ambari-server/src/main/java/org/apache/ambari/server/stack/StackServiceDirectory.java (lines 116 - 117) <https://reviews.apache.org/r/45169/#comment198003> Maybe LOG.info() here. If nothing happens we'll have no record unless DEBUG is enabled (which is, admittedly, an awful idea) ambari-server/src/main/resources/stacks/HDP/2.3/upgrades/upgrade-2.3.xml (line 166) <https://reviews.apache.org/r/45169/#comment197999> Changing this name may (or may not) impact UI. The group name is not guaranteed unique, and some additional information may be displayed based on it's name (unconfirmed, but that's the intent). Also, make consistent using an '_': SERVICE_CHECK_1 - Nate Cole On May 16, 2016, 2: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, 2: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 > >
