> 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 > >
