Carolyn Beeton wrote: >> -----Original Message----- >> From: [EMAIL PROTECTED] >> [mailto:[EMAIL PROTECTED] On Behalf Of >> Damian Krzeminski >> Sent: Tuesday, July 29, 2008 3:37 PM >> ... >> I do have a list of things that need to be fixed: >> >> Static methods (as in Alarm class) do not work well (not they >> are needed) in IoC/Spring application. They make testing >> depending classes harder. If any object needs to send alarm >> it needs to inject Alarm context. In that way if you need to >> write unit test for such object, you can inject it with dummy >> implementation of AlarmContext and not worry about your unit >> test sending XML/RPC. >> > > done > >> It would also be better to define raiseAlarm to take a >> variable list of String >> >> raiseAlarm(String alarmId, String... alarmParams) >> >> It's just a syntactical sugar but saves a lot of typing and >> you only need a single raiseAlarm method. It travels through >> XML/RPC as the same array as Vector so no changes on the >> server needed. >> > > done > >> I'd rather we do not catch Exception in places like this one: >> >> try { >> if (s_alarmContext == null) { >> LOG.debug("raiseAlarm: s_alarmContext has not been set"); >> LOG.error("could not raise alarm " + alarmId + " " + >> alarmParams); >> } else { >> s_alarmContext.raiseAlarm(alarmId, alarmParams); >> } >> } catch (Exception e) { >> LOG.error("caught exception in alarm handling; ignoring so >> we don't make it worse", e); } } >> >> >> - rarely is a good reason for that. >> >> And in this case it's just wrong because raiseAlarm can >> probably throw >> XmlRpc or User exceptions that should NOT be ignored. >> >> Instead of catching all exceptions alarm context should be proxied by >> BackgroundTaskInterceptor. In this way alarms notification are >> transparently sent in a separate thread in a non-blocking way. And if >> something fails, the failure can be logged without blocking >> the application. > > done. The offending Alarm.java is history now; code that wants to raise > an alarm uses the alarmContext directly. > >> Since XML/RPC URL is injected as a propery (and not >> configured dynamically) >> one does not need to use API provider pattern. You can >> configure XML/RPC >> proxy directly in *.beans.xml using XmlRpcProxyFactoryBean. >> XML/RPC Api >> provider is only needed if API url is passed in/from Java. >> > > haven't done this one yet... I'm not sure I really see the advantage, > but I will tackle it next week. (I've also changed the code to actually > use the alarm server URL instead of location manager)
That's OK - it's a minor thing. I can try doing that when accepting the patch. > > The patch is attached to http://track.sipfoundry.org/browse/XECS-920 > > Please note that I moved/renamed a file, and subversion doesn't cope > with that very well - notes in the issue give details. I use git to review accept patches. Renames are not a problem. It discovers them for you. Pretty amazing. Highly recommended. > > Damian, please review the patch and let me know if the changes are what > you had in mind. Monday holiday in Canada; I'll be back Tuesday to > address new issues. Thanks - I'll have a look. > > Thanks, > Carolyn _______________________________________________ sipx-dev mailing list [email protected] List Archive: http://list.sipfoundry.org/archive/sipx-dev Unsubscribe: http://list.sipfoundry.org/mailman/listinfo/sipx-dev
