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 28159: Alerts: Targets Should Support A Severity Level
Date Tue, 18 Nov 2014 14:05:02 GMT


> On Nov. 18, 2014, 8:51 a.m., Tom Beerbower wrote:
> > ambari-server/src/main/java/org/apache/ambari/server/controller/internal/AlertTargetResourceProvider.java,
line 333
> > <https://reviews.apache.org/r/28159/diff/1/?file=766855#file766855line333>
> >
> >     I think isEmpty() reads nicer also.  Of course you have to flip the if/else
or use !isEmpty().

That's a good point about flipping the if/else. I took another look at the if/else structure
and I think it does read cleaner if I swap them, so I did change this one out for isEmpty().


> On Nov. 18, 2014, 8:51 a.m., Tom Beerbower wrote:
> > ambari-server/src/main/java/org/apache/ambari/server/events/listeners/AlertStateChangedListener.java,
line 139
> > <https://reviews.apache.org/r/28159/diff/1/?file=766856#file766856line139>
> >
> >     I like the code to read as close to English as possible.  In this case it would
be alertStates.size() > 0 vs. !alertStates.isEmpty().  I'm not sure which reads easier.
 Either way is okay, I think.
> >     
> >     Funny, the null != alertStates bothers me way more since it is backwards from
how you would naturally say it and seems like just a holdover from C programming.
> >     
> >     I think these are really personal preferences.  Good to comment on but not sure
we need an issue for them.

Thanks for the review!

I've been doing constant-first for so long, that `foo == null` reads backwards to me :) It's
actually something that was engrained into my motor skills decades ago in order to prevent
accidental re-assignment. I can't tell you how many people used to accidentially write `if(value=null)`.
I'm sure Eclipse and IntelliJ warn you on that stuff in the modern age, but I still like constant-first.


- Jonathan


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


On Nov. 18, 2014, 12:34 a.m., Jonathan Hurley wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28159/
> -----------------------------------------------------------
> 
> (Updated Nov. 18, 2014, 12:34 a.m.)
> 
> 
> Review request for Ambari, Alejandro Fernandez, Nate Cole, and Tom Beerbower.
> 
> 
> Bugs: AMBARI-8362
>     https://issues.apache.org/jira/browse/AMBARI-8362
> 
> 
> Repository: ambari
> 
> 
> Description
> -------
> 
> Alert targets should have an optional field that represents the severity levels that
they care about. This will allow users to create different targets that are only alerted when
an alert's state has transitioned to their support criticality level. 
> 
> For example, a user may want to have 2 different email targets, "Tier-1 Support" and
"Tier-2 Support" where T1 receives WARNINGs only and T2 receives CRITICALS and UNKNOWNS.
> 
> We will extend the AlertTarget resource to provide this extra option. It will be a list
of supported criticality levels. If an empty list is specified, all alert states will be accepted
for the target.
> 
> An example of creating a target that only cares about OK and WARNING states.
> ```
> {
>   "AlertTarget": {
>     "name": "Administrators",
>     "description": "The Admins",
>     "notification_type": "EMAIL",
>     "alert_states": ["OK", "WARNING"],
>     "properties":{
>       "foo": "bar",
>       "foobar" : "baz"
>     }
>   }
> }
> ```
> 
> There was a database design choice that I had to make WRT storing a Set of enumerations.
JPA actually has a mechanism for this, but since Ambari doesn't use JPA correctly, it involves
creating a new table by hand. The other options that I considered were:
> - JSON or CSV string of the enumerations
> - A bit representing the OR'd value
> - A BLOB column that stored the serialized set
> 
> I chose the table since it allows us to leverage JPA for the persistence and retrieval
while preventing our providers or DAOs from needing specialized serialization logic.
> 
> 
> Diffs
> -----
> 
>   ambari-server/src/main/java/org/apache/ambari/server/controller/internal/AlertTargetResourceProvider.java
3029114 
>   ambari-server/src/main/java/org/apache/ambari/server/events/listeners/AlertStateChangedListener.java
c42851b 
>   ambari-server/src/main/java/org/apache/ambari/server/orm/entities/AlertTargetEntity.java
12c394d 
>   ambari-server/src/main/java/org/apache/ambari/server/upgrade/UpgradeCatalog200.java
2cbf266 
>   ambari-server/src/main/resources/Ambari-DDL-MySQL-CREATE.sql 4d15914 
>   ambari-server/src/main/resources/Ambari-DDL-Oracle-CREATE.sql e3ae87a 
>   ambari-server/src/main/resources/Ambari-DDL-Postgres-CREATE.sql 0587232 
>   ambari-server/src/main/resources/Ambari-DDL-Postgres-EMBEDDED-CREATE.sql 5605d57 
>   ambari-server/src/main/resources/Ambari-DDL-SQLServer-CREATE.sql 79caca7 
>   ambari-server/src/main/resources/Ambari-DDL-SQLServer-DROP.sql 7658b63 
>   ambari-server/src/test/java/org/apache/ambari/server/controller/internal/AlertTargetResourceProviderTest.java
7a633e7 
>   ambari-server/src/test/java/org/apache/ambari/server/upgrade/UpgradeCatalog200Test.java
be97222 
> 
> Diff: https://reviews.apache.org/r/28159/diff/
> 
> 
> Testing
> -------
> 
> mvn clean test and manual testing to ensure that the alert targets are skipped and no
notices are created.
> 
> 
> Thanks,
> 
> Jonathan Hurley
> 
>


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