You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@ambari.apache.org by Jonathan Hurley <jh...@hortonworks.com> on 2014/10/10 20:48:15 UTC
Review Request 26568: Alerts: Create REST API Endpoint for Alert
History
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26568/
-----------------------------------------------------------
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
Re: Review Request 26568: Alerts: Create REST API Endpoint for Alert
History
Posted by Jonathan Hurley <jh...@hortonworks.com>.
> On Oct. 10, 2014, 5:22 p.m., Nate Cole wrote:
> > ambari-server/src/main/java/org/apache/ambari/server/orm/entities/AlertHistoryEntity_.java, lines 36-37
> > <https://reviews.apache.org/r/26568/diff/1/?file=717592#file717592line36>
> >
> > Why not import annotations? I've noticed you do this other places, but isn't consistent with other parts of the code. Also, I think as long as the annotation StaticMetamodel is there you don't need that wonky _ character.
Whoa, that's odd; the import should be there; I'll fix that. As far as the Foo_ goes, it is convention. Here's a good rundown of @StaticMetamodel: http://docs.jboss.org/hibernate/stable/orm/topical/html/metamodelgen/MetamodelGenerator.html
> On Oct. 10, 2014, 5:22 p.m., Nate Cole wrote:
> > ambari-server/src/main/resources/key_properties.json, lines 158-161
> > <https://reviews.apache.org/r/26568/diff/1/?file=717593#file717593line158>
> >
> > On Tom's suggestion, I've been trying not to use key_properties and properties.json, instead just doing it in code. It's worked ok on my short foray into RU. It's ok that you do, but it works from code too.
Very well; registering in the RP and removing from properties.json and key_properties.json.
- Jonathan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26568/#review56224
-----------------------------------------------------------
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
>
>
Re: Review Request 26568: Alerts: Create REST API Endpoint for Alert
History
Posted by Nate Cole <nc...@hortonworks.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26568/#review56224
-----------------------------------------------------------
Ship it!
ambari-server/src/main/java/org/apache/ambari/server/orm/entities/AlertHistoryEntity_.java
<https://reviews.apache.org/r/26568/#comment96549>
Why not import annotations? I've noticed you do this other places, but isn't consistent with other parts of the code. Also, I think as long as the annotation StaticMetamodel is there you don't need that wonky _ character.
ambari-server/src/main/resources/key_properties.json
<https://reviews.apache.org/r/26568/#comment96551>
On Tom's suggestion, I've been trying not to use key_properties and properties.json, instead just doing it in code. It's worked ok on my short foray into RU. It's ok that you do, but it works from code too.
- Nate Cole
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
>
>
Re: Review Request 26568: Alerts: Create REST API Endpoint for Alert
History
Posted by Jonathan Hurley <jh...@hortonworks.com>.
> 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
>
>
Re: Review Request 26568: Alerts: Create REST API Endpoint for Alert
History
Posted by Jonathan Hurley <jh...@hortonworks.com>.
> 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.
>
> Jonathan Hurley wrote:
> 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.
>
> Tom Beerbower wrote:
> I guess you could distribute the NOT so that something like 'NOT(name=foo OR name=bar)' becomes 'name!=foo AND name!=bar', but that could get messy. I guess it's okay if we don't support it for this case for now but I think it would be better to throw an UnsupportedOperationException than to return the wrong result.
I think I would end up treating NOT like I do with ANDs and ORs, the difference being that NOT only has a single predicate that gets visited. So I would enqueue a new List to NOT and then visit the children of the NOT so that the entire statement gets wrapped and stays close to the original.
I will add this to the sorting and pagination Jira I'm creating for alerts.
- Jonathan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26568/#review56214
-----------------------------------------------------------
On Oct. 12, 2014, 12:33 a.m., Jonathan Hurley wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26568/
> -----------------------------------------------------------
>
> (Updated Oct. 12, 2014, 12:33 a.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/AlertDefinitionResourceProvider.java 1de49a7
> ambari-server/src/main/java/org/apache/ambari/server/controller/internal/AlertGroupResourceProvider.java 8c55aa7
> 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/internal/AlertTargetResourceProvider.java b0cc52b
> ambari-server/src/main/java/org/apache/ambari/server/controller/internal/DefaultProviderModule.java ab89db2
> 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/AlertDefinitionResourceProviderTest.java 78655dd
> ambari-server/src/test/java/org/apache/ambari/server/controller/internal/AlertGroupResourceProviderTest.java 771bf8a
> ambari-server/src/test/java/org/apache/ambari/server/controller/internal/AlertHistoryResourceProviderTest.java PRE-CREATION
> ambari-server/src/test/java/org/apache/ambari/server/controller/internal/AlertTargetResourceProviderTest.java b96a4f3
> 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
>
>
Re: Review Request 26568: Alerts: Create REST API Endpoint for Alert
History
Posted by Tom Beerbower <tb...@hortonworks.com>.
> On Oct. 10, 2014, 9: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.
>
> Jonathan Hurley wrote:
> 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.
I guess you could distribute the NOT so that something like 'NOT(name=foo OR name=bar)' becomes 'name!=foo AND name!=bar', but that could get messy. I guess it's okay if we don't support it for this case for now but I think it would be better to throw an UnsupportedOperationException than to return the wrong result.
> On Oct. 10, 2014, 9: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.
>
> Jonathan Hurley wrote:
> 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.
Probably okay, but I don't know for sure if there are any issues with having the singular and plural names the same.
- Tom
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26568/#review56214
-----------------------------------------------------------
On Oct. 12, 2014, 4:33 a.m., Jonathan Hurley wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26568/
> -----------------------------------------------------------
>
> (Updated Oct. 12, 2014, 4:33 a.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/AlertDefinitionResourceProvider.java 1de49a7
> ambari-server/src/main/java/org/apache/ambari/server/controller/internal/AlertGroupResourceProvider.java 8c55aa7
> 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/internal/AlertTargetResourceProvider.java b0cc52b
> ambari-server/src/main/java/org/apache/ambari/server/controller/internal/DefaultProviderModule.java ab89db2
> 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/AlertDefinitionResourceProviderTest.java 78655dd
> ambari-server/src/test/java/org/apache/ambari/server/controller/internal/AlertGroupResourceProviderTest.java 771bf8a
> ambari-server/src/test/java/org/apache/ambari/server/controller/internal/AlertHistoryResourceProviderTest.java PRE-CREATION
> ambari-server/src/test/java/org/apache/ambari/server/controller/internal/AlertTargetResourceProviderTest.java b96a4f3
> 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
>
>
Re: Review Request 26568: Alerts: Create REST API Endpoint for Alert
History
Posted by Tom Beerbower <tb...@hortonworks.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26568/#review56214
-----------------------------------------------------------
Very nice. Just a couple of comments / questions ...
ambari-server/src/main/java/org/apache/ambari/server/api/query/JpaPredicateVisitor.java
<https://reviews.apache.org/r/26568/#comment96536>
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.
ambari-server/src/main/java/org/apache/ambari/server/api/services/ClusterService.java
<https://reviews.apache.org/r/26568/#comment96537>
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.
ambari-server/src/main/java/org/apache/ambari/server/controller/AlertHistoryRequest.java
<https://reviews.apache.org/r/26568/#comment96540>
Is this class needed? Why not just pass the predicate? Are you planning on adding sort and page requests here as well?
ambari-server/src/main/java/org/apache/ambari/server/controller/AlertHistoryRequest.java
<https://reviews.apache.org/r/26568/#comment96544>
Is there a reason that this should be public and not private final? It seems like the request object should be immutable.
ambari-server/src/main/java/org/apache/ambari/server/orm/entities/AlertHistoryEntity_.java
<https://reviews.apache.org/r/26568/#comment96538>
I don't think that I've ever seen a trailing underscore for a class name before.
- Tom Beerbower
On Oct. 10, 2014, 6: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, 6: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
>
>
Re: Review Request 26568: Alerts: Create REST API Endpoint for Alert
History
Posted by Tom Beerbower <tb...@hortonworks.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26568/#review56345
-----------------------------------------------------------
Ship it!
Ship It!
- Tom Beerbower
On Oct. 12, 2014, 4:33 a.m., Jonathan Hurley wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26568/
> -----------------------------------------------------------
>
> (Updated Oct. 12, 2014, 4:33 a.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/AlertDefinitionResourceProvider.java 1de49a7
> ambari-server/src/main/java/org/apache/ambari/server/controller/internal/AlertGroupResourceProvider.java 8c55aa7
> 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/internal/AlertTargetResourceProvider.java b0cc52b
> ambari-server/src/main/java/org/apache/ambari/server/controller/internal/DefaultProviderModule.java ab89db2
> 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/AlertDefinitionResourceProviderTest.java 78655dd
> ambari-server/src/test/java/org/apache/ambari/server/controller/internal/AlertGroupResourceProviderTest.java 771bf8a
> ambari-server/src/test/java/org/apache/ambari/server/controller/internal/AlertHistoryResourceProviderTest.java PRE-CREATION
> ambari-server/src/test/java/org/apache/ambari/server/controller/internal/AlertTargetResourceProviderTest.java b96a4f3
> 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
>
>
Re: Review Request 26568: Alerts: Create REST API Endpoint for Alert
History
Posted by Jonathan Hurley <jh...@hortonworks.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26568/
-----------------------------------------------------------
(Updated Oct. 12, 2014, 12:33 a.m.)
Review request for Ambari, Nate Cole and Tom Beerbower.
Changes
-------
Thanks for the review!
- Moved Alert\* property declarations out of JSON files and into static{} blocks of their respective ResourceProviders.
- Is it wrong that the AlertHistoryResourceDefinition specifies "alert_history" for both singular and plural?
- Fixed the annotation import
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 (updated)
-----
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/AlertDefinitionResourceProvider.java 1de49a7
ambari-server/src/main/java/org/apache/ambari/server/controller/internal/AlertGroupResourceProvider.java 8c55aa7
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/internal/AlertTargetResourceProvider.java b0cc52b
ambari-server/src/main/java/org/apache/ambari/server/controller/internal/DefaultProviderModule.java ab89db2
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/AlertDefinitionResourceProviderTest.java 78655dd
ambari-server/src/test/java/org/apache/ambari/server/controller/internal/AlertGroupResourceProviderTest.java 771bf8a
ambari-server/src/test/java/org/apache/ambari/server/controller/internal/AlertHistoryResourceProviderTest.java PRE-CREATION
ambari-server/src/test/java/org/apache/ambari/server/controller/internal/AlertTargetResourceProviderTest.java b96a4f3
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