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