> On Jan. 19, 2015, 9:53 a.m., Jonathan Hurley wrote: > > ambari-server/src/main/java/org/apache/ambari/server/state/stack/UpgradePack.java, > > line 76 > > <https://reviews.apache.org/r/29956/diff/4/?file=824382#file824382line76> > > > > 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?
I started going down the Collections.reverse, but the ClusterGrouping stuff got in the way since they must always be first/last. So it's the case where I would reverse and have to go over the items anyway to find those wonk ServiceCheck things. > On Jan. 19, 2015, 9:53 a.m., Jonathan Hurley wrote: > > ambari-server/src/main/java/org/apache/ambari/server/state/UpgradeHelper.java, > > line 187 > > <https://reviews.apache.org/r/29956/diff/4/?file=824381#file824381line187> > > > > 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. Future. See comment to Tom. > On Jan. 19, 2015, 9:53 a.m., Jonathan Hurley wrote: > > ambari-server/src/main/java/org/apache/ambari/server/state/stack/upgrade/Grouping.java, > > line 56 > > <https://reviews.apache.org/r/29956/diff/4/?file=824384#file824384line56> > > > > 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`. I was trying to strike the balance of having annoying xml vs logical defaults; also follows the pattern Tom set forth with <skippable />. - Nate ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29956/#review68603 ----------------------------------------------------------- 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 > >
