----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29547/#review66645 -----------------------------------------------------------
Ship it! Mostly minor comments. One or two issues to correct before shipping. ambari-server/src/main/java/org/apache/ambari/server/controller/internal/AlertTargetValidationResourceProvider.java <https://reviews.apache.org/r/29547/#comment110210> Null pointer check on `dispatchFactory.getDispatcher(notificationType)` since there's no way to ensure that the correct dispatcher will exist. ambari-server/src/main/java/org/apache/ambari/server/controller/internal/AlertTargetValidationResourceProvider.java <https://reviews.apache.org/r/29547/#comment110211> I think for enums, we use `.name()` instead of `.toString()` ambari-server/src/main/java/org/apache/ambari/server/notifications/dispatchers/EmailDispatcher.java <https://reviews.apache.org/r/29547/#comment110212> Ambari doesn't use `.*` for imports. ambari-server/src/main/java/org/apache/ambari/server/notifications/dispatchers/EmailDispatcher.java <https://reviews.apache.org/r/29547/#comment110213> Because this is method designed for testing the validity of a target, exceptions are expected. However, I think we still need to either: 1) Capture the entire exception in the result so it's available if needed or 2) Log the exception to some non-critical level like DEBUG. ambari-server/src/main/java/org/apache/ambari/server/notifications/dispatchers/EmailDispatcher.java <https://reviews.apache.org/r/29547/#comment110214> `AMBARI_DISPATCH_CREDENTIAL_USERNAME` is not a requirement for connection. Only if it's specified as part of the target properties should a `DispatchCredentials` instance be created. - Jonathan Hurley On Jan. 2, 2015, 1:06 p.m., Yurii Shylov wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/29547/ > ----------------------------------------------------------- > > (Updated Jan. 2, 2015, 1:06 p.m.) > > > Review request for Ambari, Jonathan Hurley, Nate Cole, and Tom Beerbower. > > > Bugs: AMBARI-8978 > https://issues.apache.org/jira/browse/AMBARI-8978 > > > Repository: ambari > > > Description > ------- > > During the cluster installation, the web client would like to be able to have > the administrator configure an alert target for use with that cluster. > However, because there are many properties that are used to successfully > create an AlertTarget, it's likely that the settings originally provided may > not work. > > For example, when creating an AlertTarget for SMTP, if the security or port > are not valid (or the SMTP server is restricting access to certain IP > addresses) then the target won't be able to properly use it. > > We need to be able to allow an AlertTarget to be "tested" before actually > creating it in the system. > > I propose a new endpoint off of targets that can be used to POST to. The POST > can contain all of the alert properties that would normally be found on an > AlertTarget. The difference is that no target is created; instead a status is > returned about whether the target works (and why it doesn't if it failed). > > I would suggest also altering the dispatcher interface to support a new > method; something like {{Dispatcher.testAlertTarget(...)}} which will simply > exercise the properties of the target to ensure a good connection. > > > Diffs > ----- > > > ambari-server/src/main/java/org/apache/ambari/server/api/resources/AlertTargetValidationResourceDefinition.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/api/resources/ResourceInstanceFactoryImpl.java > e55b2cb > > ambari-server/src/main/java/org/apache/ambari/server/api/services/AlertTargetValidationService.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/controller/internal/AlertTargetValidationResourceProvider.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/controller/internal/DefaultProviderModule.java > be2a9ad > > ambari-server/src/main/java/org/apache/ambari/server/controller/spi/Resource.java > 89d6837 > > ambari-server/src/main/java/org/apache/ambari/server/notifications/NotificationDispatcher.java > 10946be > > ambari-server/src/main/java/org/apache/ambari/server/notifications/dispatchers/EmailDispatcher.java > f979c03 > > ambari-server/src/main/java/org/apache/ambari/server/notifications/dispatchers/SNMPDispatcher.java > 0e75801 > > ambari-server/src/main/java/org/apache/ambari/server/state/services/AlertNoticeDispatchService.java > b085112 > > ambari-server/src/test/java/org/apache/ambari/server/controller/internal/AlertTargetValidationResourceProviderTest.java > PRE-CREATION > > ambari-server/src/test/java/org/apache/ambari/server/notifications/EmailDispatcherTest.java > 1e7689f > > ambari-server/src/test/java/org/apache/ambari/server/notifications/MockDispatcher.java > 616551f > > ambari-server/src/test/java/org/apache/ambari/server/notifications/dispatchers/EmailDispatcherTest.java > PRE-CREATION > > ambari-server/src/test/java/org/apache/ambari/server/notifications/dispatchers/SNMPDispatcherTest.java > db4af1c > > ambari-server/src/test/java/org/apache/ambari/server/state/services/AlertNoticeDispatchServiceTest.java > 2e984bf > > Diff: https://reviews.apache.org/r/29547/diff/ > > > Testing > ------- > > In progress > > > Thanks, > > Yurii Shylov > >
