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 4:54:52 PM Subject: Re: [Engine-devel] [backend] a little confusion about the quartz jobs On 02/23/2012 05:23 PM, Laszlo Hornyak wrote: > > ----- Original Message ----- >> 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 understand your decision but... > - the methods are usually about 5-10 lines below the schedule.*Job > call, it is very hard not to notice the connection > - for safe and easy refactoring, it could be better to pass over a > callback Callback will introduce some limitations (I don't need to tell what are the limitations of anonymous inner classes :) ) > - plus in the schedule.*Job call it could then be better to check > if such method still exists, should throw an > IllegalArgumentException if not there in this case we could catch > the problem right at the cause, not when scheduled That depends on what point you start scheduling - do we schedule all our jobs on startup?

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