----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29956/#review68603 -----------------------------------------------------------
Ship it! ambari-server/src/main/java/org/apache/ambari/server/state/UpgradeHelper.java <https://reviews.apache.org/r/29956/#comment112927> Could we do a switch here instead where the subclass returns its type? That way you could also have default behavior instead of adding another empty else to indicate that something went wrong. ambari-server/src/main/java/org/apache/ambari/server/state/UpgradeHelper.java <https://reviews.apache.org/r/29956/#comment112928> Maybe an `else` here to log/exception that the type didn't match and there's probably a problem? ambari-server/src/main/java/org/apache/ambari/server/state/stack/UpgradePack.java <https://reviews.apache.org/r/29956/#comment112923> Doc that this method does some index kung-fu to reverse groups in a particular way. I'm also wondering if this could be simplified a little. Instead of trying to build the reverse list, could you instead just `Collections.reverse` the original, and then walked the reversed list, swapping indexes when you encounter a service group? Why special case ClusterGrouping? ambari-server/src/main/java/org/apache/ambari/server/state/stack/upgrade/Grouping.java <https://reviews.apache.org/r/29956/#comment112926> It's probably part of the requirements, but I dislike `true` elements that cause major behavioral changes. I suppose I'd rather see this be `false` and have the XML more verbose and explicit in stating when a `service-check` is to be run. But the requirements probably said they always have to run, so it was easier to make it `true`. - Jonathan Hurley On Jan. 17, 2015, 8:52 a.m., Nate Cole wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/29956/ > ----------------------------------------------------------- > > (Updated Jan. 17, 2015, 8:52 a.m.) > > > Review request for Ambari, Alejandro Fernandez, Jonathan Hurley, and Tom > Beerbower. > > > Bugs: AMBARI-9167 > https://issues.apache.org/jira/browse/AMBARI-9167 > > > Repository: ambari > > > Description > ------- > > Enable full smoke tests. Involved changes to the upgrade pack: > > Each group runs it's own service check. Need to be able to turn them off > such that the next group (a service-check one) that runs full checks doesn't > re-run ones it just did. For example. Full service checks happen after each > of CORE_MASTER, CORE_SLAVE, and CLIENT. No need to run service checks on > CORE_MASTER when running them all right after anyway. > > > Diffs > ----- > > > ambari-server/src/main/java/org/apache/ambari/server/state/UpgradeHelper.java > cdecac6 > > ambari-server/src/main/java/org/apache/ambari/server/state/stack/UpgradePack.java > 1662baa > > ambari-server/src/main/java/org/apache/ambari/server/state/stack/upgrade/ColocatedGrouping.java > 1b957da > > ambari-server/src/main/java/org/apache/ambari/server/state/stack/upgrade/Grouping.java > b4a75ae > > ambari-server/src/main/java/org/apache/ambari/server/state/stack/upgrade/ServiceCheckGrouping.java > PRE-CREATION > ambari-server/src/main/resources/stacks/HDP/2.2/upgrades/upgrade-2.2.xml > 325df21 > > ambari-server/src/test/java/org/apache/ambari/server/controller/AmbariManagementControllerTest.java > 26e1546 > > ambari-server/src/test/java/org/apache/ambari/server/state/UpgradeHelperTest.java > cf2de93 > > ambari-server/src/test/java/org/apache/ambari/server/state/stack/UpgradePackTest.java > cf191d5 > > ambari-server/src/test/resources/stacks/HDP/2.1.1/upgrades/upgrade_test_checks.xml > PRE-CREATION > > Diff: https://reviews.apache.org/r/29956/diff/ > > > Testing > ------- > > [INFO] > ------------------------------------------------------------------------ > [INFO] BUILD SUCCESS > [INFO] > ------------------------------------------------------------------------ > [INFO] Total time: 24:47.403s > [INFO] Finished at: Thu Jan 15 21:44:24 EST 2015 > [INFO] Final Memory: 31M/337M > [INFO] > ------------------------------------------------------------------------ > > > Thanks, > > Nate Cole > >
