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

[Yair Zaslavsky]

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