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

Laszlo Hornyak lhornyak at redhat.com
Thu Feb 23 16:25:22 UTC 2012 


----- Original Message -----
> From: "Yair Zaslavsky" <yzaslavs at redhat.com>
> To: "Laszlo Hornyak" <lhornyak at redhat.com>
> Cc: engine-devel at 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 at redhat.com>
> >> To: "Laszlo Hornyak" <lhornyak at redhat.com>
> >> Cc: engine-devel at 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 at redhat.com>
> >>>> To: engine-devel at 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 at ovirt.org
> >>>>>> http://lists.ovirt.org/mailman/listinfo/engine-devel
> >>>>>>
> >>>>> _______________________________________________
> >>>>> Engine-devel mailing list
> >>>>> Engine-devel at ovirt.org
> >>>>> http://lists.ovirt.org/mailman/listinfo/engine-devel
> >>>>
> >>>> _______________________________________________
> >>>> Engine-devel mailing list
> >>>> Engine-devel at ovirt.org
> >>>> http://lists.ovirt.org/mailman/listinfo/engine-devel
> >>>>
> >>
> >>
> 
>

More information about the Devel mailing list