> On June 12, 2014, 10:39 a.m., Jonathan Hurley wrote: > > ambari-server/src/main/java/org/apache/ambari/server/state/Alert.java, line > > 51 > > <https://reviews.apache.org/r/22488/diff/1/?file=607864#file607864line51> > > > > The switch statement on getState() assumes not-null. I'd do a null > > check here and keep AlertState.UNKNOWN if the passed in state was null. > > Nate Cole wrote: > The passed Alert class defaults to UNKNOWN. Since this is a temporary > bridge, I think it's ok to assume non-null.
OK - since it's temporary I think it's fine to leave it. I just saw a possibility for a NPE. > On June 12, 2014, 10:39 a.m., Jonathan Hurley wrote: > > ambari-server/src/main/java/org/apache/ambari/server/state/Alert.java, line > > 136 > > <https://reviews.apache.org/r/22488/diff/1/?file=607864#file607864line136> > > > > For equality checks, would just comparing Alert.class == o.class be > > better? isInstance(...) will return true on subclasses as well I think and > > that might lead to side effects if we don't want subclasses being equal to > > a parent instance. > > Nate Cole wrote: > Even subclasses must meet these conditions if equals() is not overridden. But this would still allow FooAlert to be equal to BarAlert even though the classes are different. If this is desired, then there isn't an issue. > On June 12, 2014, 10:39 a.m., Jonathan Hurley wrote: > > ambari-server/src/main/java/org/apache/ambari/server/state/cluster/ClusterImpl.java, > > line 1581 > > <https://reviews.apache.org/r/22488/diff/1/?file=607867#file607867line1581> > > > > I agree that clusterAlters needs a ReadWriteLock; but here we're using > > the cluster's ReadWriteLock. > > > > The clusterAlerts data structure could have its own in this case and be > > isolated from cluster locking. > > Nate Cole wrote: > I agree, but that doesn't follow how we do locking elsewhere (not that > those are correct either). What about using a CopyOnWriteArraySet or > something? Then you'd incur a penalty every time you write to it which could be a lot given the number of potential alerts. I like locking better, I just didn't see the need to use the cluster's lock since this data structure is separate. I figured you could just do a new ReadWriteLock for alerts. But if we want to keep with convention, then the code can be left as-is. - Jonathan ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/22488/#review45496 ----------------------------------------------------------- On June 11, 2014, 9:15 p.m., Nate Cole wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/22488/ > ----------------------------------------------------------- > > (Updated June 11, 2014, 9:15 p.m.) > > > Review request for Ambari, Jonathan Hurley and Sid Wagle. > > > Bugs: AMBARI-6092 > https://issues.apache.org/jira/browse/AMBARI-6092 > > > Repository: ambari > > > Description > ------- > > Add the ability for agents (in this case, Flume) to return Alert information > to Ambari. > * Only reports alerts and shows them in the UI. Another JIRA will actually > get the alerts to Nagios for distribution. > * This is the first step of a much larger design with eventual removal of > Nagios collecting alerts, so this patch may get refactored as we go, but is a > good first step. > > > Diffs > ----- > > ambari-agent/src/main/python/ambari_agent/ActionQueue.py 9defc7d > ambari-agent/src/test/python/ambari_agent/TestActionQueue.py 5c433f9 > ambari-server/src/main/java/org/apache/ambari/server/agent/AgentAlert.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/agent/ComponentStatus.java > 292247f > > ambari-server/src/main/java/org/apache/ambari/server/agent/HeartBeatHandler.java > 0f4acd7 > > ambari-server/src/main/java/org/apache/ambari/server/controller/nagios/NagiosAlert.java > ba637c2 > > ambari-server/src/main/java/org/apache/ambari/server/controller/nagios/NagiosPropertyProvider.java > ba7309a > ambari-server/src/main/java/org/apache/ambari/server/state/Alert.java > PRE-CREATION > ambari-server/src/main/java/org/apache/ambari/server/state/AlertState.java > PRE-CREATION > ambari-server/src/main/java/org/apache/ambari/server/state/Cluster.java > e1a3e42 > > ambari-server/src/main/java/org/apache/ambari/server/state/cluster/ClusterImpl.java > f7e31b2 > > ambari-server/src/main/resources/stacks/HDP/2.0.6/services/FLUME/package/scripts/flume_handler.py > 19d4afa > > ambari-server/src/main/resources/stacks/HDP/2.0.6/services/NAGIOS/package/templates/hadoop-servicegroups.cfg.j2 > 49e8fb0 > > ambari-server/src/main/resources/stacks/HDP/2.0.6/services/NAGIOS/package/templates/hadoop-services.cfg.j2 > 8ab3f7a > > ambari-server/src/test/java/org/apache/ambari/server/agent/TestHeartbeatHandler.java > 090aa81 > > ambari-server/src/test/java/org/apache/ambari/server/controller/nagios/NagiosPropertyProviderTest.java > b16457d > ambari-server/src/test/python/stacks/2.0.6/FLUME/test_flume.py d0df762 > > Diff: https://reviews.apache.org/r/22488/diff/ > > > Testing > ------- > > [INFO] > ------------------------------------------------------------------------ > [INFO] BUILD SUCCESS > [INFO] > ------------------------------------------------------------------------ > [INFO] Total time: 14:54.202s > [INFO] Finished at: Wed Jun 11 18:24:40 EDT 2014 > [INFO] Final Memory: 21M/123M > [INFO] > ------------------------------------------------------------------------ > > > Thanks, > > Nate Cole > >
