> On Nov. 18, 2014, 9:09 a.m., Nate Cole wrote: > > ambari-server/src/main/java/org/apache/ambari/server/controller/internal/AlertTargetResourceProvider.java, > > lines 328-331 > > <https://reviews.apache.org/r/28159/diff/1/?file=766855#file766855line328> > > > > This is an assumption that a caller may not realize (an empty list > > means "do them all"). Consider an Exception instead for those who would do > > ill.
Thanks for the review and a nice point to mention. Jeff and I spoke about this exact issue and we felt that since this is an optional parameter, neglecting to specify it should have the same effect as what would have happened before this change went in. Not specifying the value should mean that the target is a regular target, interested in all alert states. > On Nov. 18, 2014, 9:09 a.m., Nate Cole wrote: > > ambari-server/src/main/resources/Ambari-DDL-SQLServer-DROP.sql, lines > > 171-172 > > <https://reviews.apache.org/r/28159/diff/1/?file=766864#file766864line171> > > > > Since there's a FK, I'm not sure if this needs to be removed before > > alert_target. I don't understand SQLServer enough to know I think you're right; dropping it before alert_target. Also, I'm not sure that this is the best way to drop tables in SQLServer; I'd think they could do a query to get the tables that they'd need to drop. - Jonathan ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28159/#review61920 ----------------------------------------------------------- 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 > >
