From add7b2f8068641bed7a56a489cfc937443517c19 Mon Sep 17 00:00:00 2001 From: dsyer Date: Thu, 28 Feb 2008 15:44:51 +0000 Subject: [PATCH] OPEN - issue BATCH-378: RepeatListener is confusing and too generic to use for 'intercepting' a step http://jira.springframework.org/browse/BATCH-378 Change contract of StepListener - JobParameters are more useful at that stage than ExecutionContext. --- .../batch/core/domain/StepExecution.java | 43 ++++++++++++------- .../batch/core/domain/StepListener.java | 19 ++++++-- .../interceptor/CompositeStepListener.java | 19 ++++---- .../core/interceptor/StepListenerSupport.java | 4 +- .../CompositeStepListenerTests.java | 8 ++-- .../batch/execution/step/TaskletStep.java | 8 ++-- .../execution/step/TaskletStepTests.java | 3 +- .../batch/repeat/ExitStatus.java | 23 +++++----- .../batch/repeat/ExitStatusTests.java | 16 +++---- 9 files changed, 84 insertions(+), 59 deletions(-) diff --git a/spring-batch-core/src/main/java/org/springframework/batch/core/domain/StepExecution.java b/spring-batch-core/src/main/java/org/springframework/batch/core/domain/StepExecution.java index 5807d7fdd..9a12bf02e 100644 --- a/spring-batch-core/src/main/java/org/springframework/batch/core/domain/StepExecution.java +++ b/spring-batch-core/src/main/java/org/springframework/batch/core/domain/StepExecution.java @@ -37,7 +37,7 @@ import org.springframework.util.Assert; public class StepExecution extends Entity { private JobExecution jobExecution; - + private Step step; private BatchStatus status = BatchStatus.STARTING; @@ -47,9 +47,9 @@ public class StepExecution extends Entity { private int commitCount = 0; private int rollbackCount = 0; - + private int skipCount = 0; - + private int retryCount = 0; private Date startTime = new Date(System.currentTimeMillis()); @@ -258,7 +258,7 @@ public class StepExecution extends Entity { * @see org.springframework.batch.container.common.domain.Entity#equals(java.lang.Object) */ public boolean equals(Object obj) { - //TODO make sure the equality makes sense + // TODO make sure the equality makes sense Object jobExecutionId = getJobExecutionId(); if (step == null && jobExecutionId == null || !(obj instanceof StepExecution) || getId() == null) { return super.equals(obj); @@ -268,7 +268,7 @@ public class StepExecution extends Entity { return jobExecutionId.equals(other.getJobExecutionId()); } return step.getName().equals(other.getStepName()) - && (jobExecutionId == null || jobExecutionId.equals(other.getJobExecutionId())); + && (jobExecutionId == null || jobExecutionId.equals(other.getJobExecutionId())); } /* @@ -279,12 +279,12 @@ public class StepExecution extends Entity { public int hashCode() { Object jobExecutionId = getJobExecutionId(); return super.hashCode() + 31 * (step.getName() != null ? step.getName().hashCode() : 0) + 91 - * (jobExecutionId != null ? jobExecutionId.hashCode() : 0); + * (jobExecutionId != null ? jobExecutionId.hashCode() : 0); } public String toString() { - return super.toString() + ", name=" + step.getName() + ", taskCount=" + taskCount + ", commitCount=" + commitCount - + ", rollbackCount=" + rollbackCount; + return super.toString() + ", name=" + step.getName() + ", taskCount=" + taskCount + ", commitCount=" + + commitCount + ", rollbackCount=" + rollbackCount; } /** @@ -305,7 +305,7 @@ public class StepExecution extends Entity { * Accessor for the execution context information of the enclosing job. * * @return the {@link JobExecution} that was used to start this step - * execution. + * execution. */ public JobExecution getJobExecution() { return jobExecution; @@ -357,25 +357,38 @@ public class StepExecution extends Entity { public void setTerminateOnly() { this.terminateOnly = true; } - + public void setSkipCount(int skipCount) { this.skipCount = skipCount; } - + public int getSkipCount() { return skipCount; } - - public void incrementSkipCount(){ + + public void incrementSkipCount() { skipCount++; } - + public void setRetryCount(int retryCount) { this.retryCount = retryCount; } - + public int getRetryCount() { return retryCount; } + /** + * Convenience method to get the current job parameters. + * + * @return the {@link JobParameters} from the enclosing job, or empty if + * that is null + */ + public JobParameters getJobParameters() { + if (jobExecution == null || jobExecution.getJobInstance() == null) { + return new JobParameters(); + } + return jobExecution.getJobInstance().getJobParameters(); + } + } diff --git a/spring-batch-core/src/main/java/org/springframework/batch/core/domain/StepListener.java b/spring-batch-core/src/main/java/org/springframework/batch/core/domain/StepListener.java index 43cee0be8..b0af45137 100644 --- a/spring-batch-core/src/main/java/org/springframework/batch/core/domain/StepListener.java +++ b/spring-batch-core/src/main/java/org/springframework/batch/core/domain/StepListener.java @@ -15,17 +15,28 @@ */ package org.springframework.batch.core.domain; -import org.springframework.batch.item.ExecutionContext; import org.springframework.batch.repeat.ExitStatus; /** * @author Lucas Ward * @author Dave Syer - * + * */ public interface StepListener { - void open(ExecutionContext executionContext); - + /** + * Initialise the state of the listener with the job parameters from the + * current scope. + * @param jobParameters + */ + void open(JobParameters jobParameters); + + /** + * Give a listener a chance to modify the exit status from a step. The value + * returned will be combined with the normal exit status. + * + * @return an {@link ExitStatus} to combine with the normal value. Return + * null to leave the old value unchanged. + */ ExitStatus close(); } diff --git a/spring-batch-core/src/main/java/org/springframework/batch/core/interceptor/CompositeStepListener.java b/spring-batch-core/src/main/java/org/springframework/batch/core/interceptor/CompositeStepListener.java index 2c9722669..cf71b2200 100644 --- a/spring-batch-core/src/main/java/org/springframework/batch/core/interceptor/CompositeStepListener.java +++ b/spring-batch-core/src/main/java/org/springframework/batch/core/interceptor/CompositeStepListener.java @@ -20,8 +20,8 @@ import java.util.Arrays; import java.util.Iterator; import java.util.List; +import org.springframework.batch.core.domain.JobParameters; import org.springframework.batch.core.domain.StepListener; -import org.springframework.batch.item.ExecutionContext; import org.springframework.batch.repeat.ExitStatus; /** @@ -63,27 +63,26 @@ public class CompositeStepListener implements StepListener { } } - /* - * (non-Javadoc) + /* (non-Javadoc) * @see org.springframework.batch.core.domain.StepListener#close() */ public ExitStatus close() { - ExitStatus status = ExitStatus.CONTINUABLE; + ExitStatus status = null; for (Iterator iterator = listeners.iterator(); iterator.hasNext();) { StepListener listener = (StepListener) iterator.next(); - status = status.and(listener.close()); + ExitStatus close = listener.close(); + status = status!=null ? status.and(close): close; } return status; } - /* - * (non-Javadoc) - * @see org.springframework.batch.core.domain.StepListener#open(org.springframework.batch.item.ExecutionContext) + /* (non-Javadoc) + * @see org.springframework.batch.core.domain.StepListener#open(org.springframework.batch.core.domain.JobParameters) */ - public void open(ExecutionContext executionContext) { + public void open(JobParameters jobParameters) { for (Iterator iterator = listeners.iterator(); iterator.hasNext();) { StepListener listener = (StepListener) iterator.next(); - listener.open(executionContext); + listener.open(jobParameters); } } diff --git a/spring-batch-core/src/main/java/org/springframework/batch/core/interceptor/StepListenerSupport.java b/spring-batch-core/src/main/java/org/springframework/batch/core/interceptor/StepListenerSupport.java index 20a606d32..4b5474ea6 100644 --- a/spring-batch-core/src/main/java/org/springframework/batch/core/interceptor/StepListenerSupport.java +++ b/spring-batch-core/src/main/java/org/springframework/batch/core/interceptor/StepListenerSupport.java @@ -15,8 +15,8 @@ */ package org.springframework.batch.core.interceptor; +import org.springframework.batch.core.domain.JobParameters; import org.springframework.batch.core.domain.StepListener; -import org.springframework.batch.item.ExecutionContext; import org.springframework.batch.repeat.ExitStatus; /** @@ -35,7 +35,7 @@ public class StepListenerSupport implements StepListener { /* (non-Javadoc) * @see org.springframework.batch.core.domain.StepListener#open(org.springframework.batch.item.ExecutionContext) */ - public void open(ExecutionContext executionContext) { + public void open(JobParameters jobParameters) { } } diff --git a/spring-batch-core/src/test/java/org/springframework/batch/core/interceptor/CompositeStepListenerTests.java b/spring-batch-core/src/test/java/org/springframework/batch/core/interceptor/CompositeStepListenerTests.java index c7824226d..156e60824 100644 --- a/spring-batch-core/src/test/java/org/springframework/batch/core/interceptor/CompositeStepListenerTests.java +++ b/spring-batch-core/src/test/java/org/springframework/batch/core/interceptor/CompositeStepListenerTests.java @@ -20,8 +20,8 @@ import java.util.List; import junit.framework.TestCase; +import org.springframework.batch.core.domain.JobParameters; import org.springframework.batch.core.domain.StepListener; -import org.springframework.batch.item.ExecutionContext; import org.springframework.batch.repeat.ExitStatus; /** @@ -71,15 +71,15 @@ public class CompositeStepListenerTests extends TestCase { /** * Test method for - * {@link org.springframework.batch.core.interceptor.CompositeStepListener#open(org.springframework.batch.item.ExecutionContext)}. + * {@link org.springframework.batch.core.interceptor.CompositeStepListener#open(JobParameters)}. */ public void testOpen() { listener.setListener(new StepListenerSupport() { - public void open(ExecutionContext executionContext) { + public void open(JobParameters jobParameters) { list.add("foo"); } }); - listener.open(new ExecutionContext()); + listener.open(new JobParameters()); assertEquals(1, list.size()); } diff --git a/spring-batch-execution/src/main/java/org/springframework/batch/execution/step/TaskletStep.java b/spring-batch-execution/src/main/java/org/springframework/batch/execution/step/TaskletStep.java index 7f2483b52..8a618797c 100644 --- a/spring-batch-execution/src/main/java/org/springframework/batch/execution/step/TaskletStep.java +++ b/spring-batch-execution/src/main/java/org/springframework/batch/execution/step/TaskletStep.java @@ -180,7 +180,7 @@ public class TaskletStep implements Step, InitializingBean, BeanNameAware { Exception fatalException = null; try { - listener.open(stepExecution.getExecutionContext()); + listener.open(stepExecution.getJobParameters()); exitStatus = tasklet.execute(); try { @@ -204,14 +204,14 @@ public class TaskletStep implements Step, InitializingBean, BeanNameAware { throw new BatchCriticalException(e); } finally { - stepExecution.setExitStatus(exitStatus); - stepExecution.setEndTime(new Date()); try { - listener.close(); + exitStatus = exitStatus.and(listener.close()); } catch (Exception e) { logger.error("Encountered an error on listener close."); } + stepExecution.setExitStatus(exitStatus); + stepExecution.setEndTime(new Date()); try { jobRepository.saveOrUpdate(stepExecution); } diff --git a/spring-batch-execution/src/test/java/org/springframework/batch/execution/step/TaskletStepTests.java b/spring-batch-execution/src/test/java/org/springframework/batch/execution/step/TaskletStepTests.java index a31fa43fc..08e158889 100644 --- a/spring-batch-execution/src/test/java/org/springframework/batch/execution/step/TaskletStepTests.java +++ b/spring-batch-execution/src/test/java/org/springframework/batch/execution/step/TaskletStepTests.java @@ -17,7 +17,6 @@ import org.springframework.batch.execution.job.JobSupport; import org.springframework.batch.execution.scope.StepSynchronizationManager; import org.springframework.batch.execution.step.support.JobRepositorySupport; import org.springframework.batch.io.exception.BatchCriticalException; -import org.springframework.batch.item.ExecutionContext; import org.springframework.batch.repeat.ExitStatus; public class TaskletStepTests extends TestCase { @@ -106,7 +105,7 @@ public class TaskletStepTests extends TestCase { public void testSuccessfulExecutionWithListener() throws Exception { TaskletStep step = new TaskletStep(new StubTasklet(false, false), new JobRepositorySupport()); step.setListener(new StepListenerSupport() { - public void open(ExecutionContext context) { + public void open(JobParameters context) { list.add("open"); } public ExitStatus close() { diff --git a/spring-batch-infrastructure/src/main/java/org/springframework/batch/repeat/ExitStatus.java b/spring-batch-infrastructure/src/main/java/org/springframework/batch/repeat/ExitStatus.java index 344ab85a4..ab8f08224 100644 --- a/spring-batch-infrastructure/src/main/java/org/springframework/batch/repeat/ExitStatus.java +++ b/spring-batch-infrastructure/src/main/java/org/springframework/batch/repeat/ExitStatus.java @@ -126,14 +126,18 @@ public class ExitStatus implements Serializable { /** * Create a new {@link ExitStatus} with a logical combination of the - * continuable flag, and a con. + * continuable flag, and a concatenation of the codes. If the input is null + * we just return this. * * @param status an {@link ExitStatus} to combine with this one. * @return a new {@link ExitStatus} with {@link #isContinuable()} the * logical and of the current value and the argument provided. */ public ExitStatus and(ExitStatus status) { - return and(status.continuable).addExitCode(status.exitCode).addExitDescription(status.exitDescription); + if (status == null) { + return this; + } + return and(status.continuable).replaceExitCode(status.exitCode).addExitDescription(status.exitDescription); } /* @@ -168,16 +172,13 @@ public class ExitStatus implements Serializable { /** * Add an exit code to an existing {@link ExitStatus}. If there is already - * a code present the two will be concatenated with a semicolon. + * a code present tit will be replaced. * * @param code the code to add * @return a new {@link ExitStatus} with the same properties but a new exit * code. */ - public ExitStatus addExitCode(String code) { - if (StringUtils.hasText(exitCode) && StringUtils.hasLength(code) && !exitCode.equals(code)) { - code = exitCode + "; " + code; - } + public ExitStatus replaceExitCode(String code) { return new ExitStatus(continuable, code, exitDescription); } @@ -191,15 +192,17 @@ public class ExitStatus implements Serializable { } /** - * Add an exit description to an existing {@link ExitStatus}. If there is already - * a description present the two will be concatenated with a semicolon. + * Add an exit description to an existing {@link ExitStatus}. If there is + * already a description present the two will be concatenated with a + * semicolon. * * @param description the description to add * @return a new {@link ExitStatus} with the same properties but a new exit * description */ public ExitStatus addExitDescription(String description) { - if (StringUtils.hasText(exitDescription) && StringUtils.hasText(description) &&!exitDescription.equals(description)) { + if (StringUtils.hasText(exitDescription) && StringUtils.hasText(description) + && !exitDescription.equals(description)) { description = exitDescription + "; " + description; } return new ExitStatus(continuable, exitCode, description); diff --git a/spring-batch-infrastructure/src/test/java/org/springframework/batch/repeat/ExitStatusTests.java b/spring-batch-infrastructure/src/test/java/org/springframework/batch/repeat/ExitStatusTests.java index e729efc74..551a5663c 100644 --- a/spring-batch-infrastructure/src/test/java/org/springframework/batch/repeat/ExitStatusTests.java +++ b/spring-batch-infrastructure/src/test/java/org/springframework/batch/repeat/ExitStatusTests.java @@ -101,21 +101,21 @@ public class ExitStatusTests extends TestCase { } public void testAddExitCode() throws Exception { - ExitStatus status = ExitStatus.CONTINUABLE.addExitCode("FOO"); + ExitStatus status = ExitStatus.CONTINUABLE.replaceExitCode("FOO"); assertTrue(ExitStatus.CONTINUABLE!=status); assertTrue(status.isContinuable()); - assertEquals("CONTINUABLE; FOO", status.getExitCode()); + assertEquals("FOO", status.getExitCode()); } public void testAddExitCodeToExistingStatus() throws Exception { - ExitStatus status = ExitStatus.CONTINUABLE.addExitCode("FOO").addExitCode("BAR"); + ExitStatus status = ExitStatus.CONTINUABLE.replaceExitCode("FOO").replaceExitCode("BAR"); assertTrue(ExitStatus.CONTINUABLE!=status); assertTrue(status.isContinuable()); - assertEquals("CONTINUABLE; FOO; BAR", status.getExitCode()); + assertEquals("BAR", status.getExitCode()); } public void testAddExitCodeToSameStatus() throws Exception { - ExitStatus status = ExitStatus.CONTINUABLE.addExitCode(ExitStatus.CONTINUABLE.getExitCode()); + ExitStatus status = ExitStatus.CONTINUABLE.replaceExitCode(ExitStatus.CONTINUABLE.getExitCode()); assertTrue(ExitStatus.CONTINUABLE!=status); assertTrue(status.isContinuable()); assertEquals(ExitStatus.CONTINUABLE.getExitCode(), status.getExitCode()); @@ -136,8 +136,8 @@ public class ExitStatusTests extends TestCase { } public void testAddExitCodeWithDescription() throws Exception { - ExitStatus status = new ExitStatus(true, "BAR", "Bar").addExitCode("FOO"); - assertEquals("BAR; FOO", status.getExitCode()); + ExitStatus status = new ExitStatus(true, "BAR", "Bar").replaceExitCode("FOO"); + assertEquals("FOO", status.getExitCode()); assertEquals("Bar", status.getExitDescription()); } @@ -151,7 +151,7 @@ public class ExitStatusTests extends TestCase { } public void testSerializable() throws Exception { - ExitStatus status = ExitStatus.CONTINUABLE.addExitCode("FOO"); + ExitStatus status = ExitStatus.CONTINUABLE.replaceExitCode("FOO"); byte[] bytes = SerializationUtils.serialize(status); Object object = SerializationUtils.deserialize(bytes); assertTrue(object instanceof ExitStatus);