> On Oct. 17, 2014, 11:25 a.m., Tom Beerbower wrote: > > ambari-server/src/main/java/org/apache/ambari/server/controller/spi/Request.java, > > lines 80-103 > > <https://reviews.apache.org/r/26823/diff/1/?file=723311#file723311line80> > > > > I'd prefer to not have the setters in the interface, if possible.
I can remove the setters by moving them into the RequestImpl and using PropertyHelper to set them. > On Oct. 17, 2014, 11:25 a.m., Tom Beerbower wrote: > > ambari-server/src/main/java/org/apache/ambari/server/api/query/QueryImpl.java, > > lines 466-472 > > <https://reviews.apache.org/r/26823/diff/1/?file=723304#file723304line466> > > > > 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. Yes, I think I can move these in there. > On Oct. 17, 2014, 11:25 a.m., Tom Beerbower wrote: > > ambari-server/src/main/java/org/apache/ambari/server/controller/internal/ClusterControllerImpl.java, > > lines 211-213 > > <https://reviews.apache.org/r/26823/diff/1/?file=723309#file723309line211> > > > > 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? Yes, that's what I was thinking. It should only put the ResourceProviderPageResponse on the request if it was able to handle it itself. > On Oct. 17, 2014, 11:25 a.m., Tom Beerbower wrote: > > ambari-server/src/main/java/org/apache/ambari/server/controller/spi/ResourceProviderPageResponse.java, > > line 25 > > <https://reviews.apache.org/r/26823/diff/1/?file=723312#file723312line25> > > > > typo **presence** Fixed. > On Oct. 17, 2014, 11:25 a.m., Tom Beerbower wrote: > > ambari-server/src/main/java/org/apache/ambari/server/controller/spi/Request.java, > > lines 105-129 > > <https://reviews.apache.org/r/26823/diff/1/?file=723311#file723311line105> > > > > 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. What about the case where a PageRequest is null but there is a SortRequest? In that case, we would not be able to indicate that the ResourceProvider did the sorting on its own and the ClusterController would try to sort it again, potentially changing a correct ordering. That's why I put it on the Request. Thoughts? - Jonathan ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26823/#review57140 ----------------------------------------------------------- On Oct. 16, 2014, 5: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, 5: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 > >
