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