Mailing-List: contact dev-help@ambari.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@ambari.apache.org
Content-Type: multipart/alternative;
 boundary="===============1249382475525818695=="
MIME-Version: 1.0
Subject: Re: Review Request 28336: Provide stage resource information via REST
 API
From: "Tom Beerbower" <tbeerbower@hortonworks.com>
To: "John Speidel" <jspeidel@hortonworks.com>,
 "Nate Cole" <ncole@hortonworks.com>, "Robert Levas" <rlevas@hortonworks.com>,
 "Jonathan Hurley" <jhurley@hortonworks.com>
Cc: "Ambari" <dev@ambari.apache.org>,
 "Tom Beerbower" <tbeerbower@hortonworks.com>
Date: Sat, 22 Nov 2014 14:21:49 -0000
Message-ID: <20141122142149.16171.23735@reviews.apache.org>
Auto-Submitted: auto-generated
Sender: "Tom Beerbower" <noreply@reviews.apache.org>
References: <20141121192120.15325.46677@reviews.apache.org>
In-Reply-To: <20141121192120.15325.46677@reviews.apache.org>
Reply-To: "Tom Beerbower" <tbeerbower@hortonworks.com>

--===============1249382475525818695==
MIME-Version: 1.0
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 7bit


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


--===============1249382475525818695==--