----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28759/#review64060 -----------------------------------------------------------
Looks good. A handful of comments... mostly minor stuff or things that we can cleanup in a follow up Jira. ambari-server/src/main/java/org/apache/ambari/server/api/services/UpgradeGroupService.java <https://reviews.apache.org/r/28759/#comment106456> unused import? ambari-server/src/main/java/org/apache/ambari/server/controller/internal/RequestResourceProvider.java <https://reviews.apache.org/r/28759/#comment106433> It looks like you create and populate PROPERTY_IDS but never use it in this class. Should you refactor the constructor to use PROPERTY_IDS instead of taking a property set? ambari-server/src/main/java/org/apache/ambari/server/controller/internal/UpgradeGroupResourceProvider.java <https://reviews.apache.org/r/28759/#comment106453> Why don't you like autoboxing? ambari-server/src/main/java/org/apache/ambari/server/controller/internal/UpgradeGroupResourceProvider.java <https://reviews.apache.org/r/28759/#comment106446> don't need boxing ... new Integer(0) ambari-server/src/main/java/org/apache/ambari/server/controller/internal/UpgradeGroupResourceProvider.java <https://reviews.apache.org/r/28759/#comment106448> don't need unboxing ... percent.doubleValue() ambari-server/src/main/java/org/apache/ambari/server/controller/internal/UpgradeGroupResourceProvider.java <https://reviews.apache.org/r/28759/#comment106447> don't need boxing ... Double.valueOf(total/count) ambari-server/src/main/java/org/apache/ambari/server/controller/internal/UpgradeItemResourceProvider.java <https://reviews.apache.org/r/28759/#comment106474> Would be good if we could extend StageResourceProvider but we can look at that later. ambari-server/src/main/java/org/apache/ambari/server/controller/internal/UpgradeItemResourceProvider.java <https://reviews.apache.org/r/28759/#comment106477> hmmm... just thinking out loud. Would it be so bad to leave them as stage properties or do we really want to hide the fact that an upgrade item is a stage? ambari-server/src/main/java/org/apache/ambari/server/controller/internal/UpgradeItemResourceProvider.java <https://reviews.apache.org/r/28759/#comment106483> I think that this is fine for now but I'd like to get away from the property map / request stuff and use the JPA stuff that came in with alerts as the new model for resource providers. We can take another look at this with the follow up. Probably good to mention in a follow up Jira. ambari-server/src/main/java/org/apache/ambari/server/controller/internal/UpgradeResourceProvider.java <https://reviews.apache.org/r/28759/#comment106488> Same as with the upgrade items / stages... could we just leave them as request properties on the upgrade resource? ambari-server/src/main/java/org/apache/ambari/server/orm/dao/UpgradeDAO.java <https://reviews.apache.org/r/28759/#comment106436> missing descriptions ambari-server/src/main/java/org/apache/ambari/server/orm/dao/UpgradeDAO.java <https://reviews.apache.org/r/28759/#comment106437> is this annotation required? ambari-server/src/main/java/org/apache/ambari/server/orm/dao/UpgradeDAO.java <https://reviews.apache.org/r/28759/#comment106438> is this annotation required? ambari-server/src/main/java/org/apache/ambari/server/state/UpgradeHelper.java <https://reviews.apache.org/r/28759/#comment106457> should be clusterName ambari-server/src/main/java/org/apache/ambari/server/state/UpgradeHelper.java <https://reviews.apache.org/r/28759/#comment106461> Yeah, probably not optimal. Can we open another Jira to revisit so we don't lose track of this. ambari-server/src/main/java/org/apache/ambari/server/state/UpgradeHelper.java <https://reviews.apache.org/r/28759/#comment106467> should this be getStageResources (plural) since you are returning a set of resources? ambari-server/src/main/java/org/apache/ambari/server/state/UpgradeHelper.java <https://reviews.apache.org/r/28759/#comment106470> I think ... Request request = PropertyHelper.getReadRequest(); looks better. Could even inline it in the getResources call. ambari-server/src/main/java/org/apache/ambari/server/state/UpgradeHelper.java <https://reviews.apache.org/r/28759/#comment106471> Optional : you could use a predicate builder ... new PredicateBuilder().property(StageResourceProvider.STAGE_CLUSTER_NAME).equals(clusterName).toPredicate() In general, I think using the builder makes it read more like English but in this case I'm not sure it buys you anything. Also, don't want to put too much into these methods if it's likely that we will refactor them with the follow up. ambari-server/src/main/java/org/apache/ambari/server/state/UpgradeHelper.java <https://reviews.apache.org/r/28759/#comment106465> clusterName ambari-server/src/main/java/org/apache/ambari/server/state/UpgradeHelper.java <https://reviews.apache.org/r/28759/#comment106464> Same as above. Another Jira to follow up would be good, I think. - Tom Beerbower On Dec. 5, 2014, 4:56 p.m., Nate Cole wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/28759/ > ----------------------------------------------------------- > > (Updated Dec. 5, 2014, 4:56 p.m.) > > > Review request for Ambari, Alejandro Fernandez and Tom Beerbower. > > > Bugs: AMBARI-8539 > https://issues.apache.org/jira/browse/AMBARI-8539 > > > Repository: ambari > > > Description > ------- > > Ties Upgrade->Request and UpgradeItem->Stage more closely. The properties of > the Request/Stage are pulled in, but assigned to the Upgrade/UpgradeItem > response objects. This was preferred over having 2 "top level" objects > (which didn't work with predicates anyway). > > The method of pulling in the other objects is a bit unorthodox, and should be > revisited when we have time. > > > Diffs > ----- > > > ambari-server/src/main/java/org/apache/ambari/server/api/resources/ResourceInstanceFactoryImpl.java > b5fe94e > > ambari-server/src/main/java/org/apache/ambari/server/api/services/UpgradeGroupService.java > da21658 > > ambari-server/src/main/java/org/apache/ambari/server/api/services/UpgradeItemService.java > fb77853 > > ambari-server/src/main/java/org/apache/ambari/server/api/services/UpgradeService.java > 8b66491 > > ambari-server/src/main/java/org/apache/ambari/server/controller/internal/RequestResourceProvider.java > 561f5d9 > > ambari-server/src/main/java/org/apache/ambari/server/controller/internal/StageResourceProvider.java > 27ce015 > > ambari-server/src/main/java/org/apache/ambari/server/controller/internal/UpgradeGroupResourceProvider.java > b7eb240 > > ambari-server/src/main/java/org/apache/ambari/server/controller/internal/UpgradeItemResourceProvider.java > c481dae > > ambari-server/src/main/java/org/apache/ambari/server/controller/internal/UpgradeResourceProvider.java > a2c30f7 > > ambari-server/src/main/java/org/apache/ambari/server/orm/dao/UpgradeDAO.java > c08f809 > > ambari-server/src/main/java/org/apache/ambari/server/state/UpgradeHelper.java > 4320dbe > > ambari-server/src/test/java/org/apache/ambari/server/controller/internal/UpgradeResourceProviderTest.java > fbb4f43 > > Diff: https://reviews.apache.org/r/28759/diff/ > > > Testing > ------- > > Tests run: 2323, Failures: 0, Errors: 0, Skipped: 18 > > [INFO] > ------------------------------------------------------------------------ > [INFO] BUILD SUCCESS > [INFO] > ------------------------------------------------------------------------ > [INFO] Total time: 18:05.831s > [INFO] Finished at: Fri Dec 05 08:29:10 PST 2014 > [INFO] Final Memory: 30M/321M > [INFO] > ------------------------------------------------------------------------ > > > Thanks, > > Nate Cole > >
