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

[Moti Asayag]

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

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 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
>>>
> 
> _______________________________________________
> Engine-devel mailing list
> Engine-devel at ovirt.org
> http://lists.ovirt.org/mailman/listinfo/engine-devel