> On Oct. 3, 2014, 5:27 p.m., Nate Cole wrote: > >
Thanks for the reviews! > On Oct. 3, 2014, 5:27 p.m., Nate Cole wrote: > > ambari-server/src/main/java/org/apache/ambari/server/controller/ControllerModule.java, > > line 363 > > <https://reviews.apache.org/r/26327/diff/1/?file=713658#file713658line363> > > > > Not really an error is it? Touch call; we use EagerSingleton and if it doesn't find any (for whatever reason), then that's definitely a problem as none of the events will work. At some point in the future if we find a better way to register listeners, then we wouldn't need that annotation so it would no longer be an error. But at this point in time since it's required for major areas of Ambari to work, I make it an error. > On Oct. 3, 2014, 5:27 p.m., Nate Cole wrote: > > ambari-server/src/main/java/org/apache/ambari/server/events/AlertStateChangeEvent.java, > > line 44 > > <https://reviews.apache.org/r/26327/diff/1/?file=713659#file713659line44> > > > > Is this required anymore? You're just using the current's history > > record anyway. Nice catch - this was something I thought about as well before I committed. In theory, 2 alerts could come in for the same AlertCurrentEntity before the EventBus could handle the first event, causing the history reference that's held by AlertCurrentEntity to change. This would end with 2 AlertNotices being created with the same historical reference. Instead, we pluck the AlertHistoryEntity out at the time that the event is created so we can guarantee we'll always have the right one. > On Oct. 3, 2014, 5:27 p.m., Nate Cole wrote: > > ambari-server/src/main/java/org/apache/ambari/server/events/MaintenanceModeEvent.java, > > lines 141-143 > > <https://reviews.apache.org/r/26327/diff/1/?file=713661#file713661line141> > > > > Naming Nit: the return is a HostComponent, but the name says > > getComponent(). Changed. - Jonathan ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26327/#review55395 ----------------------------------------------------------- On Oct. 3, 2014, 5: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, 5: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 > >
