----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60113/#review178108 -----------------------------------------------------------
ambari-server/src/test/java/org/apache/ambari/server/state/services/AlertNoticeDispatchServiceTest.java Lines 39 (patched) <https://reviews.apache.org/r/60113/#comment251908> Unused import - Attila Doroszlai On June 16, 2017, 4:39 p.m., Dmytro Sen wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/60113/ > ----------------------------------------------------------- > > (Updated June 16, 2017, 4:39 p.m.) > > > Review request for Ambari, Aravindan Vijayan, Sid Wagle, and Vitalyi > Brodetskyi. > > > Bugs: AMBARI-21248 > https://issues.apache.org/jira/browse/AMBARI-21248 > > > Repository: ambari > > > Description > ------- > > In org.apache.ambari.server.state.services.AlertNoticeDispatchService, we > have below code snippet: > > > ... > protected void runOneIteration() throws Exception { > … > String targetType = target.getNotificationType(); > NotificationDispatcher dispatcher = > m_dispatchFactory.getDispatcher(targetType); > … > if (dispatcher.isDigestSupported()) { > ... > {code} > If the database has a change to AlertTargetEntity's targetType column > (unlikely but let's assume it happened), then dispatcher can be null and the > subsequent if clause will throw an exception. In > https://github.com/google/guava/blob/master/guava/src/com/google/common/util/concurrent/AbstractScheduledService.java, > we see that the exception may happen silently since shutDown() and > notifyFailed() does not log anything. > ... > {code} > class Task implements Runnable { > @Override > public void run() { > lock.lock(); > try { > if (runningTask.isCancelled()) { // task may have been cancelled > while blocked on the lock. return; } > > AbstractScheduledService.this.runOneIteration(); > } catch (Throwable t) { > try { > shutDown(); > } catch (Exception ignored) { > logger.log(Level.WARNING, "Error while attempting to shut > down the service after failure.", ignored); > } > > notifyFailed(t); > runningTask.cancel(false); // prevent future invocations. > } finally { > lock.unlock(); > } > > } > } > } > > > So, runOneIteration will shutdown the scheduler if it hits an uncaught > exception. We should wrap a try/catch. > > > Diffs > ----- > > > ambari-server/src/main/java/org/apache/ambari/server/state/services/AlertNoticeDispatchService.java > 2972f51 > > ambari-server/src/test/java/org/apache/ambari/server/state/services/AlertNoticeDispatchServiceTest.java > 42ee366 > > > Diff: https://reviews.apache.org/r/60113/diff/2/ > > > Testing > ------- > > Unit tests passed > > > Thanks, > > Dmytro Sen > >
