ambari-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Jonathan Hurley" <jhur...@hortonworks.com>
Subject Re: Review Request 26327: Alerts: Maintenance Mode Should Prevent Alert Notifications
Date Fri, 03 Oct 2014 23:42:35 GMT


> 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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message