ambari-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Jonathan Hurley" <jhur...@hortonworks.com>
Subject Re: Review Request 25071: Alerts: Create Group and Target REST Endpoints
Date Wed, 27 Aug 2014 00:15:09 GMT


> On Aug. 26, 2014, 3:51 p.m., Tom Beerbower wrote:
> > ambari-server/src/main/java/org/apache/ambari/server/api/services/AlertingService.java,
line 42
> > <https://reviews.apache.org/r/25071/diff/1/?file=669512#file669512line42>
> >
> >     I think that there is an issue here that /alerting/...  doesn't fit the basic
format that we use for the API.  Basically ...
> >     
> >         /api/v1/[plural resource type]/[resource id]/[plural sub-resource type]/[sub-resource
id]/...
> >     
> >     Would it be possible to change from ...
> >     
> >         .../alerting/groups/{id}
> >         .../alerting/targets/{id}
> >     
> >     to ...
> >     
> >         .../alert_groups/{id}
> >         .../alert_targets/{id}
> >     
> >     ? Or is there some reason that we need the extra level?  I think that we already
have an alert_definition resource.  Wouldn't alert_group and alert_target be consistent with
that?
> 
> Jonathan Hurley wrote:
>     The original idea was to have more than just groups exposed under "v1/clusters/{cluster}/alerting";
we'd also have history exposed there as well. But I don't see any reason why we couldn't just
keep consistent here and have alert_groups instead (and alert_history later on).
>     
>     Targets are a little difference since they are not bound to a cluster. In this case,
I think it makes sense to put them under "alerting"; if we want to be a little more consistent
and have a plural name, we could make this v1/alerts/targets (and later on v1/alerts/history
for cross-cluster history). Thoughts on this versus v1/alert_targets ?
> 
> Tom Beerbower wrote:
>     I think that v1/alerts/targets and v1/alerts/history still have the same issue of
not fitting the format.
>     
>     We have other resources that appear at both the cluster and root levels so it shouldn't
matter if targets are bound to a cluster or not.  Could it be ...
>     
>     v1/alert_targets and v1/alert_histories ?
>     
>     Otherwise, what would the response be for v1/alerts?  Would they get a set of alerts
back or is that just a category to group targets and histories?
> 
> Jonathan Hurley wrote:
>     I'd say it would be a category to group targets and history (and potentially anything
else that will fall under cross-cluster alerts). Requesting /v1/alerts could return the current
alert states of all clusters, for example. It's not something that's in the specification
since it's a post code complete concern, but we were trying to plan ahead for it.
>     
>     I see your point about it not fitting the model. I'll defer to you on v1/alerts vs
v1/alert_targets & v1/alert_histories & v1/alerts_current. If you think it's best
to follow the convention, I'll make the appropriate change.
> 
> Tom Beerbower wrote:
>     Ok, but I'm still open to discussion if what I suggested doesn't meet the requirements
or somehow feels wrong.
>     
>     It seems to me like v1/alerts would return all of the root level (cross-cluster)
alerts, if there is such a thing.  But then you should be able to ask for an individual root
level alert with v1/alerts/{id}.  I don't know if that makes sense.  Also, if targets and
histories are sub-resources of alerts then I would expect to be able to ask for all of the
targets or histories of a specific alert... v1/alerts/{id}/targets.  Again, I don't know if
that makes sense.  Should you be able to access a cross-cluster alert by id?  If so, then
maybe the hierarchy makes sense.

I can see there being the need for an endpoint like v1/alerts that gets back all current alerts
across clusters. This would provide an easy way to see cross-cluster alert state with a single
call. However, you make a good point about what the sub-resources would look like. I don't
think that v1/alerts/{id} makes sense.

My thought is to change the endpoint to be consistent for now and I can open up a discussion
with others about how other types of data might fit in.


- Jonathan


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


On Aug. 26, 2014, 2:25 p.m., Jonathan Hurley wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25071/
> -----------------------------------------------------------
> 
> (Updated Aug. 26, 2014, 2:25 p.m.)
> 
> 
> Review request for Ambari, Nate Cole and Tom Beerbower.
> 
> 
> Bugs: AMBARI-7021
>     https://issues.apache.org/jira/browse/AMBARI-7021
> 
> 
> Repository: ambari
> 
> 
> Description
> -------
> 
> Add the initial REST endpoints for interacting with alert targets and groups. 
> 
> Update is not implemented since there's the larger issue of how to tackle generic concepts
like target properties. Also, discussion needs to be around what default data gets returned
in the queries (like targets and definitions) - right now the minimum data is returned.
> 
> 
> Diffs
> -----
> 
>   ambari-agent/src/main/python/ambari_agent/alerts/base_alert.py 07987d91c19a13c7a582d506945bde607c74bb81

>   ambari-agent/src/main/python/ambari_agent/alerts/port_alert.py d06d1f40daeb57a7db848a465382221e765c067c

>   ambari-server/src/main/java/org/apache/ambari/server/agent/HeartBeatHandler.java dca3bd92eedfc08cfd10ffe1056ca32327e12220

>   ambari-server/src/main/java/org/apache/ambari/server/api/resources/AlertGroupResourceDefinition.java
PRE-CREATION 
>   ambari-server/src/main/java/org/apache/ambari/server/api/resources/AlertTargetResourceDefinition.java
PRE-CREATION 
>   ambari-server/src/main/java/org/apache/ambari/server/api/resources/ResourceInstanceFactoryImpl.java
13fff1d5d9d12cc1b0fa3ea62a773005a280e4d9 
>   ambari-server/src/main/java/org/apache/ambari/server/api/services/AlertGroupService.java
PRE-CREATION 
>   ambari-server/src/main/java/org/apache/ambari/server/api/services/AlertingService.java
PRE-CREATION 
>   ambari-server/src/main/java/org/apache/ambari/server/api/services/ClusterService.java
8043b3fb567b48eff682d07f978f678099d4ebc2 
>   ambari-server/src/main/java/org/apache/ambari/server/controller/AmbariServer.java 21c9d80732b05c0b09206ceb9578a23a36aa6e14

>   ambari-server/src/main/java/org/apache/ambari/server/controller/internal/AbstractControllerResourceProvider.java
71ddc8d9f6579a05e82536943d25665537558f29 
>   ambari-server/src/main/java/org/apache/ambari/server/controller/internal/AlertGroupResourceProvider.java
PRE-CREATION 
>   ambari-server/src/main/java/org/apache/ambari/server/controller/internal/AlertTargetResourceProvider.java
PRE-CREATION 
>   ambari-server/src/main/java/org/apache/ambari/server/controller/spi/Resource.java b10b4bca27d2a52bd0730f5c3a2380dffcb54ff5

>   ambari-server/src/main/java/org/apache/ambari/server/orm/dao/AlertDispatchDAO.java
2871e62f92c9767ad7b8006dd552861642896b83 
>   ambari-server/src/main/java/org/apache/ambari/server/state/alert/AlertGroup.java PRE-CREATION

>   ambari-server/src/main/java/org/apache/ambari/server/state/alert/AlertTarget.java PRE-CREATION

>   ambari-server/src/main/java/org/apache/ambari/server/state/alert/TargetType.java PRE-CREATION

>   ambari-server/src/main/resources/key_properties.json 06ebb615d7d5256db432ca012b97608cd948ebe6

>   ambari-server/src/main/resources/properties.json bc2ad22691e24e339ad1b7ffe9b1d7bf3dca8306

>   ambari-server/src/test/java/org/apache/ambari/server/controller/internal/AlertGroupResourceProviderTest.java
PRE-CREATION 
>   ambari-server/src/test/java/org/apache/ambari/server/controller/internal/AlertTargetResourceProviderTest.java
PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/25071/diff/
> 
> 
> Testing
> -------
> 
> 
> Results :
> 
> Tests run: 1945, Failures: 0, Errors: 0, Skipped: 14
> 
> 
> Thanks,
> 
> Jonathan Hurley
> 
>


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