Re: [Engine-devel] [backend] a little confusion about the quartz jobs

From: "Yair Zaslavsky" <yzaslavs(a)redhat.com&gt; To: "Laszlo Hornyak" <lhornyak(a)redhat.com&gt; Cc: engine-devel(a)ovirt.org Sent: Thursday, February 23, 2012 1:31:03 PM Subject: Re: [Engine-devel] [backend] a little confusion about the quartz jobs On 02/14/2012 06:49 PM, Laszlo Hornyak wrote: > > > ----- Original Message ----- >> From: "Yair Zaslavsky" <yzaslavs(a)redhat.com&gt; >> To: engine-devel(a)ovirt.org >> Sent: Tuesday, February 14, 2012 2:01:41 PM >> Subject: Re: [Engine-devel] [backend] a little confusion about the >> quartz jobs >> >> On 02/14/2012 02:21 PM, Mike Kolesnik wrote: >>>> hi, >>>> >>>> I was playing with the quartz jobs in the backend and I thought >>>> this >>>> is an area where some simplification and/or cleanup would be >>>> useful. >>>> >>>> - SchedulerUtil interface would be nice to hide quartz from the >>>> rest >>>> of the code, but it very rarely used, the clients are bound to >>>> it's >>>> single implementation, SchedulerUtilQuartzImpl through it's >>>> getInstance() method. >>> >>> I think the whole class name is misleading, since usually when I >>> imagine a utils class, it's a simple class that does some menial >>> work for me in static methods, and not really calls anything else >>> or even has an instance. >> +1 > > Agreed, I will rename it. > >>> >>> Maybe the class can be renamed to just Scheduler, or >>> ScheduleManager which will be more precise. >>> >>>> - It was designed to be a local EJB, Backend actually expects >>>> it >>>> to >>>> be injected. (this field is not used) >>>> - when scheduling a job, you call schedule...Job(Object >>>> instance, >>>> String methodName, ...) however, it is not the _methodname_ >>>> that >>>> the executor will look for >>>> - instead, it will check the OnTimerMethodAnnotation on all the >>>> methods. But this annotation has everywhere the methodName as >>>> value >>>> - JobWrapper actually iterates over all the methods to find the >>>> one >>>> with the right annotation >>>> >>>> So a quick simplification could be: >>>> - The annotation is not needed, it could be removed >>>> - JobWrapper could just getMethod(methodName, argClasses) >>>> instead >>>> of >>>> looking for the annotation in all of the methods >>> >>> Sounds good, or maybe just keep the annotation and not the method >>> name in the call/annotation since then if the method name changes >>> it won't break and we can easily locate all jobs by searching for >>> the annotation.. This is why the annotations were introduced in the first place, we have too much places in code where we rely on usage of strings and reflection , so if a method name gets changed, the code stops working after being compiled. As this is the case, we should consider sticking to @OnTimer annotation, but maybe a proper documentation on the motivation for it should be added.

>>> >>>> - I am really not for factoryes, but if we want to separate the >>>> interface from the implementation, then probably a >>>> SchedulerUtilFactory could help here. The dummy implementation >>>> would do just the very same thing as the >>>> SchedulerUtilQuartzImpl.getInstance() >>>> - I would remove the reference to SchedulerUtil from Backend as >>>> well, since it is not used. Really _should_ the Backend class >>>> do >>>> any scheduling? >>> >>> Backend does schedule at least one job in it's Initialize() >>> method.. >> Yes, we have the DbUsers cache manager that performs periodic >> checks >> for >> db users against AD/IPA. >> This scheduler should start upon @PostConstruct (or any logical >> equivalent). >> > > Yes but I am not sure this should happen right there. All the other > service installs it's own jobs, so maybe SessionDataContainer > should do so as well. It would look more consistent. > >>> Maybe the class should be injected, but I don't know if that >>> happens so maybe that's why it's unused. >>> >>>> >>>> Please share your thoughts. >>>> >>>> Thank you, >>>> Laszlo >>>> _______________________________________________ >>>> Engine-devel mailing list >>>> Engine-devel(a)ovirt.org >>>> http://lists.ovirt.org/mailman/listinfo/engine-devel >>>> >>> _______________________________________________ >>> Engine-devel mailing list >>> Engine-devel(a)ovirt.org >>> http://lists.ovirt.org/mailman/listinfo/engine-devel >> >> _______________________________________________ >> Engine-devel mailing list >> Engine-devel(a)ovirt.org >> http://lists.ovirt.org/mailman/listinfo/engine-devel >>