----- Original Message ----- > From: "Yair Zaslavsky" <yzasl...@redhat.com> > To: "Laszlo Hornyak" <lhorn...@redhat.com> > Cc: engine-devel@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" <yzasl...@redhat.com> > >> To: "Laszlo Hornyak" <lhorn...@redhat.com> > >> Cc: engine-devel@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" <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. > > > > 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?
No, only some of the jobs. I am not telling that the method should be checked on backend start, I am telling that the method name should be checked by the SchedulerUtilQuartzImpl when the schedule.*Job method is called, not when the job is scheduled to run. > > > > >> > >>>>> > >>>>>> - 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