> On Oct. 10, 2014, 5:10 p.m., Tom Beerbower wrote: > > ambari-server/src/main/java/org/apache/ambari/server/api/query/JpaPredicateVisitor.java, > > lines 225-227 > > <https://reviews.apache.org/r/26568/diff/1/?file=717581#file717581line225> > > > > This fact that this is not supported is a problem for a query string > > like '!name.in(foo,bar)'. I think that the ambari predicate would come to > > the RP as NOT(name=foo OR name=bar) and you would end up with a JPA > > predicate of name=foo OR name=bar.
CriteriaBuilder does support NOT and its variations; I just didn't implement it for alerts yet since I can't see an use cases for using NOT. > On Oct. 10, 2014, 5:10 p.m., Tom Beerbower wrote: > > ambari-server/src/main/java/org/apache/ambari/server/api/services/ClusterService.java, > > line 387 > > <https://reviews.apache.org/r/26568/diff/1/?file=717585#file717585line387> > > > > I guess that {clusterName}/alert_histories looks a little funny. Is > > that why you use {clusterName}/alert_history here? The pattern has always > > been to use the plural resource name. > > > > I'm surprised that {clusterName}/alert_history works since you > > specified 'alert_histories' as the plural name in the definition. alert_histories is funny looking. I would rather the endpoint be {clusterName}/alert_history; but in that case, what would the singular version be? I'm open to suggestions on naming of the REST endpoint and the resource type. > On Oct. 10, 2014, 5:10 p.m., Tom Beerbower wrote: > > ambari-server/src/main/java/org/apache/ambari/server/controller/AlertHistoryRequest.java, > > line 27 > > <https://reviews.apache.org/r/26568/diff/1/?file=717586#file717586line27> > > > > Is this class needed? Why not just pass the predicate? Are you > > planning on adding sort and page requests here as well? I am planning on including the pagination and sorting information on this class. > On Oct. 10, 2014, 5:10 p.m., Tom Beerbower wrote: > > ambari-server/src/main/java/org/apache/ambari/server/controller/AlertHistoryRequest.java, > > line 32 > > <https://reviews.apache.org/r/26568/diff/1/?file=717586#file717586line32> > > > > Is there a reason that this should be public and not private final? It > > seems like the request object should be immutable. I can't see a reason that it would need to be mutable; this is actually a pattern that I've followed for a while where encapsulation objects typically have public fields without setters/getters unless there is something specific that needs to be protected. With that said, I can't see the problem with changing a request after its been created. > On Oct. 10, 2014, 5:10 p.m., Tom Beerbower wrote: > > ambari-server/src/main/java/org/apache/ambari/server/orm/entities/AlertHistoryEntity_.java, > > line 37 > > <https://reviews.apache.org/r/26568/diff/1/?file=717592#file717592line37> > > > > I don't think that I've ever seen a trailing underscore for a class > > name before. This is the standard JPA Metamodel pattern; "For each managed class X in package p, a metamodel class X_ in package p is created." - Jonathan ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26568/#review56214 ----------------------------------------------------------- On Oct. 10, 2014, 2:48 p.m., Jonathan Hurley wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/26568/ > ----------------------------------------------------------- > > (Updated Oct. 10, 2014, 2:48 p.m.) > > > Review request for Ambari, Nate Cole and Tom Beerbower. > > > Bugs: AMBARI-7734 > https://issues.apache.org/jira/browse/AMBARI-7734 > > > Repository: ambari > > > Description > ------- > > Alert history is exposed as read-only. Since this data can grow without > bounds, the current API model of manipulating the entire result set in memory > is not viable long term. > > This patch will pass the Ambari Predicate down to the DAO which will then > turn the Predicate into a JPA Predicate capable of being handed to a > CriteriaQuery. This will allow JPA to do most of the work in reducing the > requested result set. > > Missing from this are pagination and sorting. That will be a separate change > coming next where the ResourceProviders are given both, and if possible, can > apply them to the result set they return. > > > Diffs > ----- > > > ambari-server/src/main/java/org/apache/ambari/server/api/query/JpaPredicateVisitor.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/api/resources/AlertHistoryResourceDefinition.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/api/resources/ResourceInstanceFactoryImpl.java > 74580a4 > > ambari-server/src/main/java/org/apache/ambari/server/api/services/AlertHistoryService.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/api/services/ClusterService.java > a56bcbf > > ambari-server/src/main/java/org/apache/ambari/server/controller/AlertHistoryRequest.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/controller/AmbariServer.java > e0de383 > > ambari-server/src/main/java/org/apache/ambari/server/controller/internal/AbstractControllerResourceProvider.java > 5810633 > > ambari-server/src/main/java/org/apache/ambari/server/controller/internal/AlertHistoryResourceProvider.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/controller/spi/Resource.java > f51375b > ambari-server/src/main/java/org/apache/ambari/server/orm/dao/AlertsDAO.java > f2161f3 > > ambari-server/src/main/java/org/apache/ambari/server/orm/entities/AlertHistoryEntity_.java > PRE-CREATION > ambari-server/src/main/resources/key_properties.json 085dc11 > ambari-server/src/main/resources/properties.json 8a0a58a > > ambari-server/src/test/java/org/apache/ambari/server/controller/internal/AlertHistoryResourceProviderTest.java > PRE-CREATION > > ambari-server/src/test/java/org/apache/ambari/server/orm/dao/AlertsDAOTest.java > a2023d8 > > Diff: https://reviews.apache.org/r/26568/diff/ > > > Testing > ------- > > [INFO] > ------------------------------------------------------------------------ > [INFO] BUILD SUCCESS > [INFO] > ------------------------------------------------------------------------ > [INFO] Total time: 23:24 min > [INFO] Finished at: 2014-10-10T14:45:44-04:00 > [INFO] Final Memory: 29M/230M > [INFO] > ------------------------------------------------------------------------ > > > Thanks, > > Jonathan Hurley > >
