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 26823: Make paging parameters available to individual resource handlers
Date Fri, 17 Oct 2014 15:25:38 GMT

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26823/#review57140
-----------------------------------------------------------


Looks good.  A few comments.


ambari-server/src/main/java/org/apache/ambari/server/api/query/QueryImpl.java
<https://reviews.apache.org/r/26823/#comment97602>

    Could we move this logic into PropertyHelper.getReadRequest() passing in the sort and
page info?  That way we can get an immutable Request object through createRequest() and we
don't have to expose the setters on the Request interface.



ambari-server/src/main/java/org/apache/ambari/server/controller/internal/ClusterControllerImpl.java
<https://reviews.apache.org/r/26823/#comment97603>

    So the resource provider needs to be able to determine whether or not it can return a
paged response.  I guess that it's safe for the RP to do the paging as long as it knows about
all of the properties in the predicate.  Is that what you are thinking?



ambari-server/src/main/java/org/apache/ambari/server/controller/internal/ClusterControllerImpl.java
<https://reviews.apache.org/r/26823/#comment97604>

    haha ... you realy dont like 'this.'



ambari-server/src/main/java/org/apache/ambari/server/controller/spi/Request.java
<https://reviews.apache.org/r/26823/#comment97606>

    I'd prefer to not have the setters in the interface, if possible.



ambari-server/src/main/java/org/apache/ambari/server/controller/spi/Request.java
<https://reviews.apache.org/r/26823/#comment97608>

    It's kind of strange to have a response in the request, I think.  I guess there's not
a great way to do it since getResources doesn't allow you to pass anything back but the set
of resources.  Would it be cleaner to bundle this up along with the PageRequest?  That way
at least all the page related info is kept together.



ambari-server/src/main/java/org/apache/ambari/server/controller/spi/ResourceProviderPageResponse.java
<https://reviews.apache.org/r/26823/#comment97611>

    typo **presence**


- Tom Beerbower


On Oct. 16, 2014, 9:03 p.m., Jonathan Hurley wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26823/
> -----------------------------------------------------------
> 
> (Updated Oct. 16, 2014, 9:03 p.m.)
> 
> 
> Review request for Ambari, Nate Cole and Tom Beerbower.
> 
> 
> Bugs: AMBARI-7817
>     https://issues.apache.org/jira/browse/AMBARI-7817
> 
> 
> Repository: ambari
> 
> 
> Description
> -------
> 
> Exposed pagination and sorting via the SPI Request so that concrete ResourceProviders
can do their own paging and sorting. The providers are responsibile for setting the returned
ResourceProviderPageResponse so that the ClusterController knows not to perform in-memory
processing of the result set.
> 
> Implemented the new framework for alert history and notices.
> 
> Currently, the Alert DAOs return 0 for the total count of items since I don't yet know
if the UI will need this information. If we determine that we want to implement it, there
are stub methods in the DAOs for getting the counts. I didn't want to incur the JPA penalty
if it wasn't needed.
> 
> 
> Diffs
> -----
> 
>   ambari-server/src/main/java/org/apache/ambari/server/api/query/JpaPredicateVisitor.java
660bae7 
>   ambari-server/src/main/java/org/apache/ambari/server/api/query/JpaSortBuilder.java
PRE-CREATION 
>   ambari-server/src/main/java/org/apache/ambari/server/api/query/QueryImpl.java 6dfdb49

>   ambari-server/src/main/java/org/apache/ambari/server/controller/AlertHistoryRequest.java
9dc6cc4 
>   ambari-server/src/main/java/org/apache/ambari/server/controller/AlertNoticeRequest.java
40c7c67 
>   ambari-server/src/main/java/org/apache/ambari/server/controller/internal/AlertHistoryResourceProvider.java
300f02f 
>   ambari-server/src/main/java/org/apache/ambari/server/controller/internal/AlertNoticeResourceProvider.java
c4a96ff 
>   ambari-server/src/main/java/org/apache/ambari/server/controller/internal/ClusterControllerImpl.java
5d1024a 
>   ambari-server/src/main/java/org/apache/ambari/server/controller/internal/RequestImpl.java
93eaf0a 
>   ambari-server/src/main/java/org/apache/ambari/server/controller/spi/Request.java 13a2856

>   ambari-server/src/main/java/org/apache/ambari/server/controller/spi/ResourceProviderPageResponse.java
PRE-CREATION 
>   ambari-server/src/main/java/org/apache/ambari/server/orm/dao/AlertDispatchDAO.java
a49a04a 
>   ambari-server/src/main/java/org/apache/ambari/server/orm/dao/AlertsDAO.java 8414765

>   ambari-server/src/test/java/org/apache/ambari/server/controller/internal/ClusterControllerImplTest.java
261c95d 
>   ambari-server/src/test/java/org/apache/ambari/server/orm/AlertDaoHelper.java e5a5638

>   ambari-server/src/test/java/org/apache/ambari/server/orm/dao/AlertDispatchDAOTest.java
430bede 
>   ambari-server/src/test/java/org/apache/ambari/server/orm/dao/AlertsDAOTest.java 5886ab4

> 
> Diff: https://reviews.apache.org/r/26823/diff/
> 
> 
> Testing
> -------
> 
> New tests written to cover alerts and ResourceProviderPageResponse.
> 
> [INFO] ------------------------------------------------------------------------
> [INFO] BUILD SUCCESS
> [INFO] ------------------------------------------------------------------------
> [INFO] Total time: 24:00 min
> [INFO] Finished at: 2014-10-16T17:03:28-04:00
> [INFO] Final Memory: 29M/239M
> [INFO] ------------------------------------------------------------------------
> 
> 
> Thanks,
> 
> Jonathan Hurley
> 
>


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