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/11/18 06:34:31 UTC
Review Request 28159: Alerts: Targets Should Support A Severity Level
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28159/
-----------------------------------------------------------
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
Re: Review Request 28159: Alerts: Targets Should Support A Severity
Level
Posted by Nate Cole <nc...@hortonworks.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28159/#review61924
-----------------------------------------------------------
Ship it!
Ship It!
- Nate Cole
On Nov. 18, 2014, 9:16 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, 9:16 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/state/alerts/AlertStateChangedEventTest.java 18b4123
> 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
>
>
Re: Review Request 28159: Alerts: Targets Should Support A Severity
Level
Posted by Jonathan Hurley <jh...@hortonworks.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28159/
-----------------------------------------------------------
(Updated Nov. 18, 2014, 9:16 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 (updated)
-----
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/state/alerts/AlertStateChangedEventTest.java 18b4123
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
Re: Review Request 28159: Alerts: Targets Should Support A Severity
Level
Posted by Jonathan Hurley <jh...@hortonworks.com>.
> On Nov. 18, 2014, 6:16 a.m., Robert Levas 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>
> >
> > Why use `Collection.size() > 0` instead of `Collection.isEmpty()`? `isEmpty()` seems a bit cleaner and easier to scan.
When > 0, I prefer not to use isEmpty() since the ! can get lost when reading the code, making it harder to find potential issues.
> On Nov. 18, 2014, 6:16 a.m., Robert Levas 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>
> >
> > `Collection.size()` vs `Collection.isEmpty()`?
When > 0, I prefer not to use isEmpty() since the ! can get lost when reading the code, making it harder to find potential issues.
- Jonathan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28159/#review61899
-----------------------------------------------------------
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
>
>
Re: Review Request 28159: Alerts: Targets Should Support A Severity
Level
Posted by Robert Levas <rl...@hortonworks.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28159/#review61899
-----------------------------------------------------------
ambari-server/src/main/java/org/apache/ambari/server/controller/internal/AlertTargetResourceProvider.java
<https://reviews.apache.org/r/28159/#comment103826>
Why use `Collection.size() > 0` instead of `Collection.isEmpty()`? `isEmpty()` seems a bit cleaner and easier to scan.
ambari-server/src/main/java/org/apache/ambari/server/events/listeners/AlertStateChangedListener.java
<https://reviews.apache.org/r/28159/#comment103827>
`Collection.size()` vs `Collection.isEmpty()`?
- Robert Levas
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
>
>
Re: Review Request 28159: Alerts: Targets Should Support A Severity
Level
Posted by Jonathan Hurley <jh...@hortonworks.com>.
> 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
>
>
Re: Review Request 28159: Alerts: Targets Should Support A Severity
Level
Posted by Tom Beerbower <tb...@hortonworks.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28159/#review61915
-----------------------------------------------------------
Ship it!
Looks good.
ambari-server/src/main/java/org/apache/ambari/server/controller/internal/AlertTargetResourceProvider.java
<https://reviews.apache.org/r/28159/#comment103841>
I think isEmpty() reads nicer also. Of course you have to flip the if/else or use !isEmpty().
ambari-server/src/main/java/org/apache/ambari/server/events/listeners/AlertStateChangedListener.java
<https://reviews.apache.org/r/28159/#comment103843>
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.
- Tom Beerbower
On Nov. 18, 2014, 5: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, 5: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
>
>
Re: Review Request 28159: Alerts: Targets Should Support A Severity
Level
Posted by Nate Cole <nc...@hortonworks.com>.
> 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
>
> Jonathan Hurley wrote:
> 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.
+1 to that.
> 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.
>
> Jonathan Hurley wrote:
> 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.
Very well :)
- Nate
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28159/#review61920
-----------------------------------------------------------
On Nov. 18, 2014, 9:16 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, 9:16 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/state/alerts/AlertStateChangedEventTest.java 18b4123
> 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
>
>
Re: Review Request 28159: Alerts: Targets Should Support A Severity
Level
Posted by Jonathan Hurley <jh...@hortonworks.com>.
> 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
>
>
Re: Review Request 28159: Alerts: Targets Should Support A Severity
Level
Posted by Nate Cole <nc...@hortonworks.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28159/#review61920
-----------------------------------------------------------
ambari-server/src/main/java/org/apache/ambari/server/controller/internal/AlertTargetResourceProvider.java
<https://reviews.apache.org/r/28159/#comment103849>
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.
ambari-server/src/main/resources/Ambari-DDL-SQLServer-DROP.sql
<https://reviews.apache.org/r/28159/#comment103852>
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
- Nate Cole
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
>
>