From e6ac1213f743e0da3077b193633429956a91be8d Mon Sep 17 00:00:00 2001 From: Michael Vorburger Date: Sun, 11 May 2014 00:37:58 +0200 Subject: [PATCH 1/5] Correct space-instead-tab and one-line formatting for new log. --- .../jobs/service/JobRegisterServiceImpl.java | 13 ++++--------- 1 file changed, 4 insertions(+), 9 deletions(-) diff --git a/mifosng-provider/src/main/java/org/mifosplatform/infrastructure/jobs/service/JobRegisterServiceImpl.java b/mifosng-provider/src/main/java/org/mifosplatform/infrastructure/jobs/service/JobRegisterServiceImpl.java index 1a41f2803..ca3f40dce 100644 --- a/mifosng-provider/src/main/java/org/mifosplatform/infrastructure/jobs/service/JobRegisterServiceImpl.java +++ b/mifosng-provider/src/main/java/org/mifosplatform/infrastructure/jobs/service/JobRegisterServiceImpl.java @@ -108,12 +108,9 @@ public class JobRegisterServiceImpl implements JobRegisterService { } } catch (final Exception e) { - final String msg = "Job execution failed for job with id:" - + scheduledJobDetail.getId(); - logger.error(msg, e); - throw new PlatformInternalServerException( - "error.msg.sheduler.job.execution.failed", msg, - scheduledJobDetail.getId()); + final String msg = "Job execution failed for job with id:" + scheduledJobDetail.getId(); + logger.error(msg, e); + throw new PlatformInternalServerException("error.msg.sheduler.job.execution.failed", msg, scheduledJobDetail.getId()); } } @@ -218,9 +215,7 @@ public class JobRegisterServiceImpl implements JobRegisterService { scheduledJobDetails.updateNextRunTime(null); final String stackTrace = getStackTraceAsString(throwable); scheduledJobDetails.updateErrorLog(stackTrace); - logger.error( - "Could not schedule job: " - + scheduledJobDetails.getJobName(), throwable); + logger.error("Could not schedule job: " + scheduledJobDetails.getJobName(), throwable); } scheduledJobDetails.updateCurrentlyRunningStatus(false); } From d33c16e5944a141fadba5aaed44f393572181605 Mon Sep 17 00:00:00 2001 From: Michael Vorburger Date: Sun, 11 May 2014 01:16:45 +0200 Subject: [PATCH 2/5] Just some clean-up to see more clearly - this should not change anything. PLEASE CAREFULLY CODE REVIEW NEVERTHELESS. --- .../jobs/service/JobRegisterServiceImpl.java | 31 ++++++------------- 1 file changed, 9 insertions(+), 22 deletions(-) diff --git a/mifosng-provider/src/main/java/org/mifosplatform/infrastructure/jobs/service/JobRegisterServiceImpl.java b/mifosng-provider/src/main/java/org/mifosplatform/infrastructure/jobs/service/JobRegisterServiceImpl.java index ca3f40dce..c71a58247 100644 --- a/mifosng-provider/src/main/java/org/mifosplatform/infrastructure/jobs/service/JobRegisterServiceImpl.java +++ b/mifosng-provider/src/main/java/org/mifosplatform/infrastructure/jobs/service/JobRegisterServiceImpl.java @@ -95,10 +95,9 @@ public class JobRegisterServiceImpl implements JobRegisterService { if (scheduler == null || !scheduler.checkExists(jobKey)) { final JobDetail jobDetail = createJobDetail(scheduledJobDetail); final String tempSchedulerName = "temp" + scheduledJobDetail.getId(); - final List> listenerClasses = new ArrayList>(2); - listenerClasses.add(SchedulerJobListener.class); - listenerClasses.add(SchedulerStopListener.class); - final Scheduler tempScheduler = createScheduler(tempSchedulerName, 1, listenerClasses); + @SuppressWarnings("unchecked") + final Scheduler tempScheduler = createScheduler(tempSchedulerName, 1, SchedulerJobListener.class, + SchedulerStopListener.class); tempScheduler.addJob(jobDetail, true); jobDataMap.put(SchedulerServiceConstants.SCHEDULER_NAME, tempSchedulerName); this.schedulers.put(tempSchedulerName, tempScheduler); @@ -220,6 +219,7 @@ public class JobRegisterServiceImpl implements JobRegisterService { scheduledJobDetails.updateCurrentlyRunningStatus(false); } + @SuppressWarnings("unchecked") private Scheduler getScheduler(final ScheduledJobDetail scheduledJobDetail) throws Exception { final String schedulername = getSchedulerName(scheduledJobDetail); Scheduler scheduler = this.schedulers.get(schedulername); @@ -228,7 +228,7 @@ public class JobRegisterServiceImpl implements JobRegisterService { if (scheduledJobDetail.getSchedulerGroup() > 0) { noOfThreads = SchedulerServiceConstants.GROUP_THREAD_COUNT; } - scheduler = createScheduler(schedulername, noOfThreads, null); + scheduler = createScheduler(schedulername, noOfThreads, SchedulerJobListener.class); this.schedulers.put(schedulername, scheduler); } return scheduler; @@ -254,15 +254,11 @@ public class JobRegisterServiceImpl implements JobRegisterService { return sb.toString(); } - private Scheduler createScheduler(final String name, final int noOfThreads, List> listenerClasses) + private Scheduler createScheduler(final String name, final int noOfThreads, Class... listenerClasses) throws Exception { final SchedulerFactoryBean schedulerFactoryBean = new SchedulerFactoryBean(); schedulerFactoryBean.setSchedulerName(name); - if (listenerClasses == null) { - listenerClasses = new ArrayList>(1); - listenerClasses.add(SchedulerJobListener.class); - } - schedulerFactoryBean.setGlobalJobListeners(getGlobalListener(listenerClasses)); + schedulerFactoryBean.setGlobalJobListeners(getGlobalListeners(listenerClasses)); final TriggerListener globalTriggerListener = this.applicationContext.getBean("schedulerTriggerListener", TriggerListener.class); final TriggerListener[] globalTriggerListeners = { globalTriggerListener }; schedulerFactoryBean.setGlobalTriggerListeners(globalTriggerListeners); @@ -274,8 +270,8 @@ public class JobRegisterServiceImpl implements JobRegisterService { return schedulerFactoryBean.getScheduler(); } - private JobListener[] getGlobalListener(final List> listenerClasses) throws ClassNotFoundException { - final List listeners = new ArrayList(listenerClasses.size()); + private JobListener[] getGlobalListeners(final Class[] listenerClasses) throws ClassNotFoundException { + final List listeners = new ArrayList(listenerClasses.length); for (final Class listenerClass : listenerClasses) { final JobListener listener = (JobListener) getBeanObject(listenerClass); listeners.add(listener); @@ -298,11 +294,6 @@ public class JobRegisterServiceImpl implements JobRegisterService { return jobDetailFactoryBean.getObject(); } - /** - * @param jobDetails - * @return - * @throws ClassNotFoundException - */ private Object getBeanObject(final Class classType) throws ClassNotFoundException { final List> typesList = new ArrayList>(); final Class[] interfaceType = classType.getInterfaces(); @@ -332,10 +323,6 @@ public class JobRegisterServiceImpl implements JobRegisterService { return targetObject; } - /** - * @param scheduledJobDetails - * @return - */ private Trigger createTrigger(final ScheduledJobDetail scheduledJobDetails, final JobDetail jobDetail) { final MifosPlatformTenant tenant = ThreadLocalContextUtil.getTenant(); final CronTriggerFactoryBean cronTriggerFactoryBean = new CronTriggerFactoryBean(); From 226439bef1399eb99d2b7c5ee3b1ca3741acdfec Mon Sep 17 00:00:00 2001 From: Michael Vorburger Date: Sun, 11 May 2014 01:48:49 +0200 Subject: [PATCH 3/5] MIFOSX-1184 FIXED JobRegisterServiceImpl <-> SchedulerStopListener circular bean dependency --- .../jobs/service/JobRegisterServiceImpl.java | 60 +++++++++++-------- .../jobs/service/SchedulerStopListener.java | 10 +++- 2 files changed, 42 insertions(+), 28 deletions(-) diff --git a/mifosng-provider/src/main/java/org/mifosplatform/infrastructure/jobs/service/JobRegisterServiceImpl.java b/mifosng-provider/src/main/java/org/mifosplatform/infrastructure/jobs/service/JobRegisterServiceImpl.java index c71a58247..10a40d49d 100644 --- a/mifosng-provider/src/main/java/org/mifosplatform/infrastructure/jobs/service/JobRegisterServiceImpl.java +++ b/mifosng-provider/src/main/java/org/mifosplatform/infrastructure/jobs/service/JobRegisterServiceImpl.java @@ -38,29 +38,50 @@ import org.springframework.stereotype.Service; * Service class to create and load batch jobs to Scheduler using * {@link SchedulerFactoryBean} ,{@link MethodInvokingJobDetailFactoryBean} and * {@link CronTriggerFactoryBean} - * */ @Service public class JobRegisterServiceImpl implements JobRegisterService { - private final static Logger logger = LoggerFactory.getLogger(JobRegisterServiceImpl.class); - private final ApplicationContext applicationContext; + // MIFOSX-1184: This class cannot use constructor injection, because one of + // its dependencies (SchedulerStopListener) has a circular dependency to + // itself. So, slightly differently from how it's done elsewhere in this + // code base, the following fields are not final, and there is no + // constructor, but setters. - private final SchedularWritePlatformService schedularWritePlatformService; - - private final TenantDetailsService tenantDetailsService; + private ApplicationContext applicationContext; + private SchedularWritePlatformService schedularWritePlatformService; + private TenantDetailsService tenantDetailsService; + private SchedulerJobListener schedulerJobListener; + private SchedulerStopListener schedulerStopListener; private final HashMap schedulers = new HashMap(4); @Autowired - public JobRegisterServiceImpl(final ApplicationContext applicationContext, final SchedularWritePlatformService schedularService, - final TenantDetailsService tenantDetailsService) { + public void setApplicationContext(ApplicationContext applicationContext) { this.applicationContext = applicationContext; - this.schedularWritePlatformService = schedularService; + } + + @Autowired + public void setSchedularWritePlatformService(SchedularWritePlatformService schedularWritePlatformService) { + this.schedularWritePlatformService = schedularWritePlatformService; + } + + @Autowired + public void setTenantDetailsService(TenantDetailsService tenantDetailsService) { this.tenantDetailsService = tenantDetailsService; } + @Autowired + public void setSchedulerJobListener(SchedulerJobListener schedulerJobListener) { + this.schedulerJobListener = schedulerJobListener; + } + + @Autowired + public void setSchedulerStopListener(SchedulerStopListener schedulerStopListener) { + this.schedulerStopListener = schedulerStopListener; + } + @PostConstruct public void loadAllJobs() { final List allTenants = this.tenantDetailsService.findAllTenants(); @@ -95,9 +116,7 @@ public class JobRegisterServiceImpl implements JobRegisterService { if (scheduler == null || !scheduler.checkExists(jobKey)) { final JobDetail jobDetail = createJobDetail(scheduledJobDetail); final String tempSchedulerName = "temp" + scheduledJobDetail.getId(); - @SuppressWarnings("unchecked") - final Scheduler tempScheduler = createScheduler(tempSchedulerName, 1, SchedulerJobListener.class, - SchedulerStopListener.class); + final Scheduler tempScheduler = createScheduler(tempSchedulerName, 1, schedulerJobListener, schedulerStopListener); tempScheduler.addJob(jobDetail, true); jobDataMap.put(SchedulerServiceConstants.SCHEDULER_NAME, tempSchedulerName); this.schedulers.put(tempSchedulerName, tempScheduler); @@ -219,7 +238,6 @@ public class JobRegisterServiceImpl implements JobRegisterService { scheduledJobDetails.updateCurrentlyRunningStatus(false); } - @SuppressWarnings("unchecked") private Scheduler getScheduler(final ScheduledJobDetail scheduledJobDetail) throws Exception { final String schedulername = getSchedulerName(scheduledJobDetail); Scheduler scheduler = this.schedulers.get(schedulername); @@ -228,7 +246,7 @@ public class JobRegisterServiceImpl implements JobRegisterService { if (scheduledJobDetail.getSchedulerGroup() > 0) { noOfThreads = SchedulerServiceConstants.GROUP_THREAD_COUNT; } - scheduler = createScheduler(schedulername, noOfThreads, SchedulerJobListener.class); + scheduler = createScheduler(schedulername, noOfThreads, schedulerJobListener); this.schedulers.put(schedulername, scheduler); } return scheduler; @@ -254,11 +272,11 @@ public class JobRegisterServiceImpl implements JobRegisterService { return sb.toString(); } - private Scheduler createScheduler(final String name, final int noOfThreads, Class... listenerClasses) + private Scheduler createScheduler(final String name, final int noOfThreads, JobListener... jobListeners) throws Exception { final SchedulerFactoryBean schedulerFactoryBean = new SchedulerFactoryBean(); schedulerFactoryBean.setSchedulerName(name); - schedulerFactoryBean.setGlobalJobListeners(getGlobalListeners(listenerClasses)); + schedulerFactoryBean.setGlobalJobListeners(jobListeners); final TriggerListener globalTriggerListener = this.applicationContext.getBean("schedulerTriggerListener", TriggerListener.class); final TriggerListener[] globalTriggerListeners = { globalTriggerListener }; schedulerFactoryBean.setGlobalTriggerListeners(globalTriggerListeners); @@ -270,16 +288,6 @@ public class JobRegisterServiceImpl implements JobRegisterService { return schedulerFactoryBean.getScheduler(); } - private JobListener[] getGlobalListeners(final Class[] listenerClasses) throws ClassNotFoundException { - final List listeners = new ArrayList(listenerClasses.length); - for (final Class listenerClass : listenerClasses) { - final JobListener listener = (JobListener) getBeanObject(listenerClass); - listeners.add(listener); - } - final JobListener[] listenerArray = new JobListener[listeners.size()]; - return listeners.toArray(listenerArray); - } - private JobDetail createJobDetail(final ScheduledJobDetail scheduledJobDetail) throws Exception { final MifosPlatformTenant tenant = ThreadLocalContextUtil.getTenant(); final String[] jobDetails = CronMethodParser.findTargetMethodDetails(scheduledJobDetail.getJobName()); diff --git a/mifosng-provider/src/main/java/org/mifosplatform/infrastructure/jobs/service/SchedulerStopListener.java b/mifosng-provider/src/main/java/org/mifosplatform/infrastructure/jobs/service/SchedulerStopListener.java index 1b3a80135..881c917d7 100755 --- a/mifosng-provider/src/main/java/org/mifosplatform/infrastructure/jobs/service/SchedulerStopListener.java +++ b/mifosng-provider/src/main/java/org/mifosplatform/infrastructure/jobs/service/SchedulerStopListener.java @@ -15,10 +15,16 @@ public class SchedulerStopListener implements JobListener { private static final String name = "Singlr Trigger Global Listner"; - private final JobRegisterService jobRegisterService; + // MIFOSX-1184: This class cannot use constructor injection, because one of + // its dependencies (SchedulerStopListener) has a circular dependency to + // itself. So, slightly differently from how it's done elsewhere in this + // code base, the following fields are not final, and there is no + // constructor, but setters. + + private JobRegisterService jobRegisterService; @Autowired - public SchedulerStopListener(final JobRegisterService jobRegisterService) { + public void setJobRegisterService(JobRegisterService jobRegisterService) { this.jobRegisterService = jobRegisterService; } From f194867c1571cd7c48b9c648119ce71d4965d2a4 Mon Sep 17 00:00:00 2001 From: Michael Vorburger Date: Sun, 11 May 2014 02:03:47 +0200 Subject: [PATCH 4/5] SchedulerTriggerListener really should just be normally injected, like everything else; it's an anti pattern to directly use applicationContext, unless there is a specific good reason for it. --- .../jobs/service/JobRegisterServiceImpl.java | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/mifosng-provider/src/main/java/org/mifosplatform/infrastructure/jobs/service/JobRegisterServiceImpl.java b/mifosng-provider/src/main/java/org/mifosplatform/infrastructure/jobs/service/JobRegisterServiceImpl.java index 10a40d49d..fe20e40f1 100644 --- a/mifosng-provider/src/main/java/org/mifosplatform/infrastructure/jobs/service/JobRegisterServiceImpl.java +++ b/mifosng-provider/src/main/java/org/mifosplatform/infrastructure/jobs/service/JobRegisterServiceImpl.java @@ -54,6 +54,7 @@ public class JobRegisterServiceImpl implements JobRegisterService { private TenantDetailsService tenantDetailsService; private SchedulerJobListener schedulerJobListener; private SchedulerStopListener schedulerStopListener; + private SchedulerTriggerListener globalSchedulerTriggerListener; private final HashMap schedulers = new HashMap(4); @@ -82,6 +83,11 @@ public class JobRegisterServiceImpl implements JobRegisterService { this.schedulerStopListener = schedulerStopListener; } + @Autowired + public void setGlobalTriggerListener(SchedulerTriggerListener globalTriggerListener) { + this.globalSchedulerTriggerListener = globalTriggerListener; + } + @PostConstruct public void loadAllJobs() { final List allTenants = this.tenantDetailsService.findAllTenants(); @@ -277,8 +283,7 @@ public class JobRegisterServiceImpl implements JobRegisterService { final SchedulerFactoryBean schedulerFactoryBean = new SchedulerFactoryBean(); schedulerFactoryBean.setSchedulerName(name); schedulerFactoryBean.setGlobalJobListeners(jobListeners); - final TriggerListener globalTriggerListener = this.applicationContext.getBean("schedulerTriggerListener", TriggerListener.class); - final TriggerListener[] globalTriggerListeners = { globalTriggerListener }; + final TriggerListener[] globalTriggerListeners = { globalSchedulerTriggerListener }; schedulerFactoryBean.setGlobalTriggerListeners(globalTriggerListeners); final Properties quartzProperties = new Properties(); quartzProperties.put(SchedulerFactoryBean.PROP_THREAD_COUNT, Integer.toString(noOfThreads)); From 37710750eba5638332eb2a635eff1b1db11e7b17 Mon Sep 17 00:00:00 2001 From: Michael Vorburger Date: Sun, 11 May 2014 02:10:00 +0200 Subject: [PATCH 5/5] ClassMethodNamesPair for CronMethodParser instead of CLASS_INDEX / METHOD_INDEX --- .../jobs/annotation/CronMethodParser.java | 20 ++++++++++--------- .../jobs/service/JobRegisterServiceImpl.java | 7 ++++--- 2 files changed, 15 insertions(+), 12 deletions(-) diff --git a/mifosng-provider/src/main/java/org/mifosplatform/infrastructure/jobs/annotation/CronMethodParser.java b/mifosng-provider/src/main/java/org/mifosplatform/infrastructure/jobs/annotation/CronMethodParser.java index ba44331d0..e1e826b47 100644 --- a/mifosng-provider/src/main/java/org/mifosplatform/infrastructure/jobs/annotation/CronMethodParser.java +++ b/mifosng-provider/src/main/java/org/mifosplatform/infrastructure/jobs/annotation/CronMethodParser.java @@ -19,27 +19,27 @@ import org.springframework.util.ClassUtils; /** * Parser to find method which is marked with CronTargetMethod annotation - * */ public class CronMethodParser { + public static class ClassMethodNamesPair { + public String className; + public String methodName; + } + private static final String SEARCH_PACKAGE = "org.mifosplatform."; private static final String CRON_ANNOTATION_ATTRIBUTE_NAME = "jobName"; private static final String RESOURCE_PATTERN = "**/*.class"; - private static final Map targetMethosMap = new HashMap(); + private static final Map targetMethosMap = new HashMap(); private static final ResourcePatternResolver resourcePatternResolver = new PathMatchingResourcePatternResolver(); private static final MetadataReaderFactory metadataReaderFactory = new CachingMetadataReaderFactory(resourcePatternResolver); - public static final int CLASS_INDEX = 0; - - public static final int METHOD_INDEX = 1; - - public static String[] findTargetMethodDetails(final String attributeValue) throws IOException { + public static ClassMethodNamesPair findTargetMethodDetails(final String attributeValue) throws IOException { if (!targetMethosMap.containsKey(attributeValue)) { findAnnotationMethods(CronTarget.class, CRON_ANNOTATION_ATTRIBUTE_NAME); } @@ -66,8 +66,10 @@ public class CronMethodParser { final Map attributes = metadata.getAnnotationAttributes(annotationClass.getName()); final JobName attributeValue = (JobName) attributes.get(attributeName); final String className = metadata.getDeclaringClassName(); - final String[] mapVal = { className, metadata.getMethodName() }; - targetMethosMap.put(attributeValue.toString(), mapVal); + final ClassMethodNamesPair pair = new ClassMethodNamesPair(); + pair.className = className; + pair.methodName = metadata.getMethodName(); + targetMethosMap.put(attributeValue.toString(), pair); } } } diff --git a/mifosng-provider/src/main/java/org/mifosplatform/infrastructure/jobs/service/JobRegisterServiceImpl.java b/mifosng-provider/src/main/java/org/mifosplatform/infrastructure/jobs/service/JobRegisterServiceImpl.java index fe20e40f1..0acac5c7e 100644 --- a/mifosng-provider/src/main/java/org/mifosplatform/infrastructure/jobs/service/JobRegisterServiceImpl.java +++ b/mifosng-provider/src/main/java/org/mifosplatform/infrastructure/jobs/service/JobRegisterServiceImpl.java @@ -13,6 +13,7 @@ import org.mifosplatform.infrastructure.core.domain.MifosPlatformTenant; import org.mifosplatform.infrastructure.core.exception.PlatformInternalServerException; import org.mifosplatform.infrastructure.core.service.ThreadLocalContextUtil; import org.mifosplatform.infrastructure.jobs.annotation.CronMethodParser; +import org.mifosplatform.infrastructure.jobs.annotation.CronMethodParser.ClassMethodNamesPair; import org.mifosplatform.infrastructure.jobs.domain.ScheduledJobDetail; import org.mifosplatform.infrastructure.jobs.domain.SchedulerDetail; import org.mifosplatform.infrastructure.jobs.exception.JobNotFoundException; @@ -295,12 +296,12 @@ public class JobRegisterServiceImpl implements JobRegisterService { private JobDetail createJobDetail(final ScheduledJobDetail scheduledJobDetail) throws Exception { final MifosPlatformTenant tenant = ThreadLocalContextUtil.getTenant(); - final String[] jobDetails = CronMethodParser.findTargetMethodDetails(scheduledJobDetail.getJobName()); - final Object targetObject = getBeanObject(Class.forName(jobDetails[CronMethodParser.CLASS_INDEX])); + final ClassMethodNamesPair jobDetails = CronMethodParser.findTargetMethodDetails(scheduledJobDetail.getJobName()); + final Object targetObject = getBeanObject(Class.forName(jobDetails.className)); final MethodInvokingJobDetailFactoryBean jobDetailFactoryBean = new MethodInvokingJobDetailFactoryBean(); jobDetailFactoryBean.setName(scheduledJobDetail.getJobName() + "JobDetail" + tenant.getId()); jobDetailFactoryBean.setTargetObject(targetObject); - jobDetailFactoryBean.setTargetMethod(jobDetails[CronMethodParser.METHOD_INDEX]); + jobDetailFactoryBean.setTargetMethod(jobDetails.methodName); jobDetailFactoryBean.setGroup(scheduledJobDetail.getGroupName()); jobDetailFactoryBean.setConcurrent(false); jobDetailFactoryBean.afterPropertiesSet();