----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26327/#review55400 -----------------------------------------------------------
Ship it! Minor comments / questions. Looks good. ambari-server/src/main/java/org/apache/ambari/server/controller/ControllerModule.java <https://reviews.apache.org/r/26327/#comment95754> Is a null return from findCandidateComponents actually possible? If so, how did you decide that null means error and not just that no matching components were found? The docs for ClassPathScanningCandidateComponentProvider aren't very helpful. ambari-server/src/main/java/org/apache/ambari/server/events/listeners/AlertAggregateListener.java <https://reviews.apache.org/r/26327/#comment95757> I know that you mentioned it in your comment, but it does seem redundant to say this object is both a singleton and an eager singleton. ambari-server/src/test/java/org/apache/ambari/server/orm/dao/AlertsDAOTest.java <https://reviews.apache.org/r/26327/#comment95760> I wonder if we should standardize on the use of m_. I used to use it but switched away since it looked like most of the group didn't. I don't don't really care either way and I understand that we each have our own style. I just like consistency across the project. - Tom Beerbower On Oct. 3, 2014, 9:01 p.m., Jonathan Hurley wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/26327/ > ----------------------------------------------------------- > > (Updated Oct. 3, 2014, 9:01 p.m.) > > > Review request for Ambari, Nate Cole and Tom Beerbower. > > > Bugs: AMBARI-7642 > https://issues.apache.org/jira/browse/AMBARI-7642 > > > Repository: ambari > > > Description > ------- > > Putting a service or a host into maintenance mode should cause all instances > of alerts that would be affected to also go into maintenance mode. > > During this mode, the database is updated to reflect that maintenance mode is > on, however alerts still remain scheduled and running on the agents. > > Data that is returned is stored as usual, but no alert state changes are > triggered. > > Some explanation of side effect changes: > - EagerSingleton is a way for us to annotate a listener that needs to be > registerd with an EventBus. Previously, ControllerModule would need to call > out each class individually and it was an emerging anti-pattern. Now, > ControllerModule just scans for EagerSingletons and binds them generically. > It actually might be possible to remove @Singleton since we are doing a > `bind(...).asEagerSingleton` which should enforce it being a real Guice > singleton; but then I figured someone might also want to key off of > @Singleton as well, so I left that annotation. > > - HostImpl and ServiceImpl had a ton of code formatting change (I _hate_ the > use of `this.` when it's not necessary). The only real changes here were to > fire the MM event. > > > Diffs > ----- > > ambari-server/src/main/java/org/apache/ambari/server/EagerSingleton.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/controller/ControllerModule.java > da78639f13e0c5877b6f715017f094e29a0c687a > > ambari-server/src/main/java/org/apache/ambari/server/events/AlertStateChangeEvent.java > efb7119a21595ee5424a4ff463861ec6d7813fec > > ambari-server/src/main/java/org/apache/ambari/server/events/AmbariEvent.java > 3dae67667413667c7266fe80e23f2f3248a185a7 > > ambari-server/src/main/java/org/apache/ambari/server/events/MaintenanceModeEvent.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/events/listeners/AlertAggregateListener.java > a0c8e467c304da4ffcaf59cf0fbb90a153ba0a70 > > ambari-server/src/main/java/org/apache/ambari/server/events/listeners/AlertLifecycleListener.java > 9d2f4c623243bebdf6b6841803fe405425cf8511 > > ambari-server/src/main/java/org/apache/ambari/server/events/listeners/AlertMaintenanceModeListener.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/events/listeners/AlertReceivedListener.java > 78d8e664755e35c73f9eff23646c3283dbe7563f > > ambari-server/src/main/java/org/apache/ambari/server/events/listeners/AlertServiceStateListener.java > 6215cc1ba6f1f12914aa25bbcc3f17c988f60406 > > ambari-server/src/main/java/org/apache/ambari/server/events/listeners/AlertStateChangedListener.java > b2f08d7ca401b6d22a6c85587e18c2ed0c3746bf > ambari-server/src/main/java/org/apache/ambari/server/state/ServiceImpl.java > e6b5921f681a80f2117ecc8457cdf8f0104b95b4 > > ambari-server/src/main/java/org/apache/ambari/server/state/host/HostImpl.java > 619859c270baa00b5780329cf5639efee9bd308f > > ambari-server/src/main/java/org/apache/ambari/server/state/svccomphost/ServiceComponentHostImpl.java > 75265b7a52d5ee562f748b3e2164a3a68a197c72 > ambari-server/src/test/java/org/apache/ambari/server/events/EventsTest.java > PRE-CREATION > > ambari-server/src/test/java/org/apache/ambari/server/events/MockEventListener.java > PRE-CREATION > > ambari-server/src/test/java/org/apache/ambari/server/orm/dao/AlertsDAOTest.java > 0cc2ae7484674db044c6639077d726cd2943874f > > ambari-server/src/test/java/org/apache/ambari/server/state/cluster/AlertDataManagerTest.java > 502c3b9be88c111aea49f537e40d1fded1fe1ae6 > > Diff: https://reviews.apache.org/r/26327/diff/ > > > Testing > ------- > > New tests added; mvn clean test > > [INFO] > ------------------------------------------------------------------------ > [INFO] BUILD SUCCESS > [INFO] > ------------------------------------------------------------------------ > [INFO] Total time: 23:04 min > [INFO] Finished at: 2014-10-03T16:47:18-04:00 > [INFO] Final Memory: 29M/220M > [INFO] > ------------------------------------------------------------------------ > > > Thanks, > > Jonathan Hurley > >
