[Engine-devel] [backend] a little confusion about the quartz jobs
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. - 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 - 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? Please share your thoughts. Thank you, Laszlo
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. 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..
- 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.. 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
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
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..
- 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).
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
----- Original Message -----
From: "Yair Zaslavsky" <yzaslavs@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..
- 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
On 02/14/2012 06:49 PM, Laszlo Hornyak wrote:
----- Original Message -----
From: "Yair Zaslavsky" <yzaslavs@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 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
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@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
----- Original Message -----
From: "Yair Zaslavsky" <yzaslavs@redhat.com> To: "Laszlo Hornyak" <lhornyak@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" <yzaslavs@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 - 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
- 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
On 02/23/2012 05:23 PM, Laszlo Hornyak wrote:
----- Original Message -----
From: "Yair Zaslavsky" <yzaslavs@redhat.com> To: "Laszlo Hornyak" <lhornyak@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" <yzaslavs@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?
- 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
----- Original Message -----
From: "Yair Zaslavsky" <yzaslavs@redhat.com> To: "Laszlo Hornyak" <lhornyak@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" <yzaslavs@redhat.com> To: "Laszlo Hornyak" <lhornyak@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" <yzaslavs@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
participants (4)
-
Laszlo Hornyak -
Mike Kolesnik -
Moti Asayag -
Yair Zaslavsky