Moti Asayag has posted comments on this change. Change subject: engine: DB persistent quartz scheduler ......................................................................
Patch Set 10: (12 comments) 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. 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 should be examined ? either it should be able to resolve the correct class by the value set on that map, or this logic should be applied for the simple scheduler for consistency. 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 single catch block 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 should extract a common behavior into SchedulerUtilBase which will be extended by SchedulerUtilQuartzImpl and PersistentSchedulerUtilQuartz. It will clarify what is the common behavior and which are the differences. Since with the current approach we're kinda hiding some common logic (i.e. setBasicMapValues). 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. 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 PersistentScheduler (not to reveal persistent mechanism) so when searching for schedulers, it is more expectable pattern "*Scheduler" 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 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 perhaps "initializeScheduler(props)" describes it better. 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 single one. 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 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/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 ? 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 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
