> On Nov. 21, 2014, 7:21 p.m., Robert Levas wrote: > >
Thanks for the review. > On Nov. 21, 2014, 7:21 p.m., Robert Levas wrote: > > ambari-server/src/main/java/org/apache/ambari/server/api/resources/BaseResourceDefinition.java, > > line 70 > > <https://reviews.apache.org/r/28336/diff/1/?file=772232#file772232line70> > > > > This seems to be confusing since the caller sees X number of > > `Resources.Type` arguments that do not indicate any hierarchy, yet the > > implementation takes the first as the main type and the rest as the sub > > types. > > > > This type of syntax seems to make better sense when all of the > > arguements are used the same way. > > > > I would rather see something like `public > > BaseResourceDefinition(Resource.Type resourceType, > > Collection<Resource.Type> subTypes)` making the invocation of this > > constructor look like: > > > > `super(Resource.Type.Request, Arrays.asList(Resource.Type.State, > > Resource.Type.Task))` > > > > Which looks clearer (to me) than: > > > > `super(Resource.Type.Request, Resource.Type.State, Resource.Type.Task)` > > > > If there are no other issues and no on else cares about this, then > > don't let this hold up progress. Same comment to Nate ... Yeah, I think looking at the ctor it makes sense but looking at the call you just see a single list of types. I got rid of StageResourceDefinition and turned it into a generic class (SimpleResourceDefinition) that also takes the singular and plural names into the ctor. Now the call looks cleaner, I think, because the type and subtypes are broken up in the argument list. > On Nov. 21, 2014, 7:21 p.m., Robert Levas wrote: > > ambari-server/src/main/java/org/apache/ambari/server/controller/internal/ClusterControllerImpl.java, > > line 908 > > <https://reviews.apache.org/r/28336/diff/1/?file=772241#file772241line908> > > > > Is there any particular reason this is an _inner_ class. Technically > > is it not really an inner class. That said, I am typically a fan of the > > `private static (inner) class`; however this is a `protected static (inner) > > class` in an _implementation_ class (not a class one would expect to be > > used as a base class). > > > > Might it be better to move this out to its own class? Maybe it will be > > useful elsewhere? Yeah, I don't see this ever being used outside ClusterControllerImpl. I changed it to private. > On Nov. 21, 2014, 7:21 p.m., Robert Levas wrote: > > ambari-server/src/main/java/org/apache/ambari/server/controller/internal/StageResourceProvider.java, > > line 294 > > <https://reviews.apache.org/r/28336/diff/1/?file=772245#file772245line294> > > > > The comment here is confusing since we are counting non-`COMPLETED` > > statuses twice. We just don't want to doubly count the `COMPLETED` statuses > > twice. > > > > It took a little digging to figure out that this is not a bug due to > > how the counts are used elsewhere in this class. Good point. I updated the comment. - Tom ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28336/#review62584 ----------------------------------------------------------- On Nov. 22, 2014, 2:18 p.m., Tom Beerbower wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/28336/ > ----------------------------------------------------------- > > (Updated Nov. 22, 2014, 2:18 p.m.) > > > Review request for Ambari, Jonathan Hurley, John Speidel, Nate Cole, and > Robert Levas. > > > Bugs: AMBARI-8163 > https://issues.apache.org/jira/browse/AMBARI-8163 > > > Repository: ambari > > > Description > ------- > > Currently, it is possible to query Ambari (via the REST API) for details > about _asynchronous_ requests and their related tasks. This useful when > trying to obtain progress information. However, some information necessary > for the UI to indicate meaningful progress is not available. This > information is related to the stages that are generated. > > *NOTE:* Each _asynchronous_ request is broken down into 1 or more stages and > each stage contains 1 or more tasks. > > If stage information was available via the REST API, it would be possible for > the caller (maybe a UI) to track high-level tasks (at the {{stage}} level) > rather than each lower-level unit of work (at the {{task}} level). > > To allow for this, a new API resource (and associated handler) needs to be > created. The resource should be read-only (like {{requests}} and {{tasks}}), > and should provide information stored in the {{stage}} table from the Ambari > database. > > The following properties should be returned for each {{stage}}: > > * stage_id > * request_id > * cluster_id > * request_context > ** _This should probably be renamed to something more appropriate, like > stage_context, stage_name, or etc..._ > * start_time > * end_time > * progress_percent > * status > > It is expected that the resources would be queried using: > > {code} > GET /api/v1/clusters/{clusterid}/requests/{requestid}/stages > {code} > > Also, some subset of the stage data should be provided when querying for > details about a specific {{request}}, like in: > > {code} > GET /api/v1/clusters/{clusterid}/requests/{requestid} > {code} > > See {{request}} and {{task}} resource for examples. > > > Diffs > ----- > > > ambari-server/src/main/java/org/apache/ambari/server/api/query/QueryImpl.java > a1c4882 > > ambari-server/src/main/java/org/apache/ambari/server/api/resources/BaseResourceDefinition.java > a5a7234 > > ambari-server/src/main/java/org/apache/ambari/server/api/resources/RequestResourceDefinition.java > 291b01a > > ambari-server/src/main/java/org/apache/ambari/server/api/resources/ResourceInstanceFactoryImpl.java > ba3f32f > > ambari-server/src/main/java/org/apache/ambari/server/api/resources/SimpleResourceDefinition.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/api/services/RequestService.java > fc1b515 > > ambari-server/src/main/java/org/apache/ambari/server/api/services/StageService.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/api/services/TaskService.java > ade8d9c > > ambari-server/src/main/java/org/apache/ambari/server/controller/internal/AlertHistoryResourceProvider.java > 409aace > > ambari-server/src/main/java/org/apache/ambari/server/controller/internal/AlertNoticeResourceProvider.java > 956f710 > > ambari-server/src/main/java/org/apache/ambari/server/controller/internal/ClusterControllerImpl.java > ec40c4f > > ambari-server/src/main/java/org/apache/ambari/server/controller/internal/DefaultProviderModule.java > d0ce1cf > > ambari-server/src/main/java/org/apache/ambari/server/controller/internal/QueryResponseImpl.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/controller/internal/RequestImpl.java > 9b98737 > > ambari-server/src/main/java/org/apache/ambari/server/controller/internal/StageResourceProvider.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/controller/spi/ClusterController.java > 35ea680 > > ambari-server/src/main/java/org/apache/ambari/server/controller/spi/ExtendedResourceProvider.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/controller/spi/QueryResponse.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/controller/spi/Request.java > 92c5db5 > > ambari-server/src/main/java/org/apache/ambari/server/controller/spi/Resource.java > b71b43c > > ambari-server/src/main/java/org/apache/ambari/server/controller/utilities/PropertyHelper.java > 3e2111e > ambari-server/src/main/java/org/apache/ambari/server/orm/dao/StageDAO.java > 900dbeb > > ambari-server/src/main/java/org/apache/ambari/server/orm/entities/StageEntity_.java > PRE-CREATION > > ambari-server/src/test/java/org/apache/ambari/server/api/resources/SimpleResourceDefinitionTest.java > PRE-CREATION > > ambari-server/src/test/java/org/apache/ambari/server/controller/internal/ClusterControllerImplTest.java > cfb2efb > > ambari-server/src/test/java/org/apache/ambari/server/controller/internal/QueryResponseImplTest.java > PRE-CREATION > > ambari-server/src/test/java/org/apache/ambari/server/controller/internal/StageResourceProviderTest.java > PRE-CREATION > > Diff: https://reviews.apache.org/r/28336/diff/ > > > Testing > ------- > > New unit tests added. > > Results : > > Tests run: 2288, Failures: 0, Errors: 0, Skipped: 14 > > > [INFO] > ------------------------------------------------------------------------ > [INFO] BUILD SUCCESS > [INFO] > ------------------------------------------------------------------------ > [INFO] Total time: 26:48 min > [INFO] Finished at: 2014-11-22T08:06:15-05:00 > [INFO] Final Memory: 41M/485M > [INFO] > ------------------------------------------------------------------------ > > > Thanks, > > Tom Beerbower > >
