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:58 GMT


> On Oct. 3, 2014, 5:58 p.m., Tom Beerbower wrote:
> > Minor comments / questions.  Looks good.

Thanks for the review!


> On Oct. 3, 2014, 5:58 p.m., Tom Beerbower wrote:
> > ambari-server/src/main/java/org/apache/ambari/server/controller/ControllerModule.java,
lines 362-364
> > <https://reviews.apache.org/r/26327/diff/1/?file=713658#file713658line362>
> >
> >     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.

The source code will return an instantiated LinkedHashSet, but I don't trust it. Unless the
documentation declares never null, if I'm going to use an enhanced-for, I like to ensure I
won't get a NPE. An update to Spring could change this behavior under the hood and we wouldn't
know about it until the NPE.

With that said, this should always return something, so I do think my if-statement is wrong.
I should be checking for null (b/c I won't sleep at night if I don't) and I should check for
a size of 0. Since this eager registration is necessary for Ambari to work, I think keeping
it a Log.error(...) is correct.


> On Oct. 3, 2014, 5:58 p.m., Tom Beerbower wrote:
> > ambari-server/src/test/java/org/apache/ambari/server/orm/dao/AlertsDAOTest.java,
lines 81-85
> > <https://reviews.apache.org/r/26327/diff/1/?file=713673#file713673line81>
> >
> >     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.

I like consistency too, which is why I wanted to start using it hoping it would catch on.
I think it's important to have for code reviews especially since it helps define the scope
of something that's not easily determined since you're outside an IDE.

I can live without hungarian notation, but I like the prefixing very much and find it useful
in reviews. Maybe we need to start an initiave in the community to decide on some stardards
like this.


> On Oct. 3, 2014, 5:58 p.m., Tom Beerbower wrote:
> > ambari-server/src/main/java/org/apache/ambari/server/events/listeners/AlertAggregateListener.java,
lines 44-45
> > <https://reviews.apache.org/r/26327/diff/1/?file=713662#file713662line44>
> >
> >     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.

The detection of @EagerSingleton will cause a singleton binding in Guice (that's good!) so
we could get rid of @Singleton. I just feel odd doing it since:
- Someone who is not familiar with our annotation might not realize that this class is a Guice
singleton.
- Someone looking for whether a class is a Guice singleton reflectively would think this wasn't.

I can ditch @Singleton if there's a consesus between ncole and yourself.


- Jonathan


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26327/#review55400
-----------------------------------------------------------


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