ambari-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Tom Beerbower" <tbeerbo...@hortonworks.com>
Subject Re: Review Request 28336: Provide stage resource information via REST API
Date Sat, 22 Nov 2014 14:21:49 GMT


> 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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message