On 02/23/2012 02:31 PM, Yair Zaslavsky wrote: > On 02/14/2012 06:49 PM, Laszlo Hornyak wrote: >> >> >> ----- Original Message ----- >>> From: "Yair Zaslavsky" <yzasl...@redhat.com> >>> To: engine-devel@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. >
Since we don't have track for the scheduled jobs on ovirt-engine, perhaps it would be better to concentrate the system jobs that currently are spread all over is a single Job's name constant file, and provide a meaningful name for each job instead the "OnTime". It will able somehow to gain control over the jobs in the system in a single point of code instead searching for the OnTimer annotation all over. >>>> >>>>> - 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@ovirt.org >>>>> http://lists.ovirt.org/mailman/listinfo/engine-devel >>>>> >>>> _______________________________________________ >>>> Engine-devel mailing list >>>> Engine-devel@ovirt.org >>>> http://lists.ovirt.org/mailman/listinfo/engine-devel >>> >>> _______________________________________________ >>> Engine-devel mailing list >>> Engine-devel@ovirt.org >>> http://lists.ovirt.org/mailman/listinfo/engine-devel >>> > > _______________________________________________ > Engine-devel mailing list > Engine-devel@ovirt.org > http://lists.ovirt.org/mailman/listinfo/engine-devel _______________________________________________ Engine-devel mailing list Engine-devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-devel