Sahina Bose has posted comments on this change. Change subject: engine: DB persistent quartz scheduler ......................................................................
Patch Set 10: (13 comments) Patchset to follow http://gerrit.ovirt.org/#/c/36297/10/backend/manager/modules/scheduler/src/main/java/org/ovirt/engine/core/utils/timer/PersistentJobWrapper.java File backend/manager/modules/scheduler/src/main/java/org/ovirt/engine/core/utils/timer/PersistentJobWrapper.java: Line 19: * given object. Line 20: * @param context Line 21: * the context for this job. Line 22: */ Line 23: @SuppressWarnings("unchecked") > i don't see why "unchecked" suppression is required here. Done Line 24: @Override Line 25: public void execute(JobExecutionContext context) throws JobExecutionException { Line 26: super.execute(context); Line 27: } Line 29: @Override Line 30: protected Object getInstanceToRun(Map paramsMap) { Line 31: String instanceName = (String) paramsMap.get(SchedulerUtilQuartzImpl.RUNNABLE_INSTANCE); Line 32: try { Line 33: Class<?> clazz = Class.forName(instanceName); > just wonder why only for DB Scheduled scheduler the existence of the class The difference is that in the Persistent scheduler, the map contains the class name and not the serialized instance of the class (as this has to be stored and retrieved from database). To get the instance, the class instance is created and returned. In the in-memory scheduler, the instance of the class is stored in the map and is retrieved directly Line 34: Constructor<?> constructor = clazz.getConstructor(); Line 35: Object instance = constructor.newInstance(); Line 36: return instance; Line 37: } catch (ClassNotFoundException e) { Line 33: Class<?> clazz = Class.forName(instanceName); Line 34: Constructor<?> constructor = clazz.getConstructor(); Line 35: Object instance = constructor.newInstance(); Line 36: return instance; Line 37: } catch (ClassNotFoundException e) { > both "catch" statements executes exactly the same - please merge into a sin Done Line 38: log.error("could not instantiate class '{}' due to error '{}'", instanceName, e.getMessage()); Line 39: log.debug("Exception", e); Line 40: return null; Line 41: } catch (NoSuchMethodException | SecurityException | InstantiationException | IllegalAccessException http://gerrit.ovirt.org/#/c/36297/10/backend/manager/modules/scheduler/src/main/java/org/ovirt/engine/core/utils/timer/SchedulerUtilQuartzDBImpl.java File backend/manager/modules/scheduler/src/main/java/org/ovirt/engine/core/utils/timer/SchedulerUtilQuartzDBImpl.java: > Since we're adding another implementation of the SchedulerUtil, IMO we shou Done Line 1: package org.ovirt.engine.core.utils.timer; Line 2: Line 3: import static org.quartz.JobBuilder.newJob; Line 4: import static org.quartz.impl.matchers.GroupMatcher.jobGroupEquals; Line 28: import org.quartz.impl.StdSchedulerFactory; Line 29: import org.slf4j.Logger; Line 30: import org.slf4j.LoggerFactory; Line 31: Line 32: // Singleton bean, named SchedulerDB. > please use /** */ for documenting class level comment. Most of this is from superclass. Removing it from here Line 33: // The @Startup annotation is to make sure the bean is initialized on startup. Line 34: // @ConcurrencyManagement - we use bean managed concurrency: Line 35: // Singletons that use bean-managed concurrency allow full concurrent access to all the Line 36: // business and timeout methods in the singleton. Line 34: // @ConcurrencyManagement - we use bean managed concurrency: Line 35: // Singletons that use bean-managed concurrency allow full concurrent access to all the Line 36: // business and timeout methods in the singleton. Line 37: // The developer of the singleton is responsible for ensuring that the state of the singleton is synchronized across all clients. Line 38: @Singleton(name = "SchedulerDB") > just thinking out loud: Maybe it is better to name it DbScheduler or Persis PersistentScheduler sounds good. changing Line 39: @DependsOn("LockManager") Line 40: @Startup Line 41: @TransactionAttribute(TransactionAttributeType.NOT_SUPPORTED) Line 42: @ConcurrencyManagement(ConcurrencyManagementType.BEAN) Line 39: @DependsOn("LockManager") Line 40: @Startup Line 41: @TransactionAttribute(TransactionAttributeType.NOT_SUPPORTED) Line 42: @ConcurrencyManagement(ConcurrencyManagementType.BEAN) Line 43: public class SchedulerUtilQuartzDBImpl extends SchedulerUtilQuartzImpl implements SchedulerUtil { > and respectively - DbSchedulerUtilQuartzImpl Done Line 44: Line 45: private final Logger log = LoggerFactory.getLogger(SchedulerUtilQuartzDBImpl.class); Line 46: Line 47: /* Line 60: } Line 61: setup(props); Line 62: } Line 63: Line 64: public void setup(Properties props) { > it seems that we initialize and start the scheduler within this method, so I added this as more to enable testing, so it's an overloaded method of the super class which takes a Properties file. Can rename this if it helps clarity. Let me know Line 65: try { Line 66: Line 67: SchedulerFactory sf = Line 68: new StdSchedulerFactory(props); Line 63: Line 64: public void setup(Properties props) { Line 65: try { Line 66: Line 67: SchedulerFactory sf = > this code should be reformatted, i.e both lines should be merged in to a si Done Line 68: new StdSchedulerFactory(props); Line 69: sched = sf.getScheduler(); Line 70: if (sched != null) { Line 71: sched.start(); http://gerrit.ovirt.org/#/c/36297/10/backend/manager/modules/utils/src/main/java/org/ovirt/engine/core/utils/ejb/BeanType.java File backend/manager/modules/utils/src/main/java/org/ovirt/engine/core/utils/ejb/BeanType.java: Line 5: */ Line 6: public enum BeanType { Line 7: BACKEND, // Backend bean Line 8: SCHEDULER, // SchedulerUtil Line 9: PERSISTENT_SCHEDULER, // SchedulerUtil > the comment is redundant Done Line 10: VDS_EVENT_LISTENER, Line 11: LOCK_MANAGER, Line 12: EVENTQUEUE_MANAGER, Line 13: CACHE_CONTAINER; http://gerrit.ovirt.org/#/c/36297/10/packaging/dbscripts/upgrade/03_06_0710_insert_quartz_tables.sql File packaging/dbscripts/upgrade/03_06_0710_insert_quartz_tables.sql: Line 12: DROP TABLE IF EXISTS qrtz_simprop_triggers CASCADE; Line 13: DROP TABLE IF EXISTS qrtz_blob_triggers CASCADE; Line 14: DROP TABLE IF EXISTS qrtz_triggers CASCADE; Line 15: DROP TABLE IF EXISTS qrtz_job_details CASCADE; Line 16: DROP TABLE IF EXISTS qrtz_calendars CASCADE; > Aren't all those new tables (as I see below) , so , why you need to drop fi I'd retained the script from the quartz packaged scripts. Is there an issue leaving these here? Otherwise will remove Line 17: Line 18: CREATE TABLE qrtz_job_details Line 19: ( Line 20: sched_name VARCHAR(120) NOT NULL, http://gerrit.ovirt.org/#/c/36297/10/packaging/services/ovirt-engine/ovirt-engine.conf.in File packaging/services/ovirt-engine/ovirt-engine.conf.in: Line 193: # Line 194: # Size of the database connection pool for non-JTA Line 195: # datasource used by Quartz Line 196: # Line 197: ENGINE_NOJTA_DB_MIN_CONNECTIONS=1 > can we break it into NON_JTA instead NOJTA ? Done Line 198: ENGINE_NOJTA_DB_MAX_CONNECTIONS=10 Line 199: Line 200: # Line 201: # Timeout value in milliseconds for stop checking if database Line 194: # Size of the database connection pool for non-JTA Line 195: # datasource used by Quartz Line 196: # Line 197: ENGINE_NOJTA_DB_MIN_CONNECTIONS=1 Line 198: ENGINE_NOJTA_DB_MAX_CONNECTIONS=10 > same Done Line 199: Line 200: # Line 201: # Timeout value in milliseconds for stop checking if database Line 202: # connectivity is available (5 minutes at the moment): -- To view, visit http://gerrit.ovirt.org/36297 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I9a34dac95999cb6b3721d201c116fb5f6089bb61 Gerrit-PatchSet: 10 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Sahina Bose <sab...@redhat.com> Gerrit-Reviewer: Eli Mesika <emes...@redhat.com> Gerrit-Reviewer: Liran Zelkha <lzel...@redhat.com> Gerrit-Reviewer: Moti Asayag <masa...@redhat.com> Gerrit-Reviewer: Oved Ourfali <oourf...@redhat.com> Gerrit-Reviewer: Sahina Bose <sab...@redhat.com> Gerrit-Reviewer: Shubhendu Tripathi <shtri...@redhat.com> Gerrit-Reviewer: Yair Zaslavsky <yzasl...@redhat.com> Gerrit-Reviewer: anmolbabu <anb...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
