Update Task to BOOT 2.1.M1 #441

cppwfs · 2018-08-15T19:38:13Z

Migrating to use ApplicationContextRunner or ImportAutoConfiguration with SpringApp.run, instead of SpringApplicationBuilder, because builder does not handle AutoConfiguration properly

SimpleTaskAutoConfiguration now has an annotation AutoConfigureBefore the BatchTaskAutoConfig so that it is processed prior. THis is so that that BatchTaskAutoConfig can create the appropriate beans

SimpleTaskAutoConfiguration has new annotations so that it is AutoConfigured after BindingServiceConfiguration and after SimpleTaskAutoConfiguration. This is so that it does not attempt to start emitting messages before stream is ready and it can create the appropriate beans after SimpleTaskAutoConfiguration has run.

Renamed SimpleTaskConfiguration to SimpleTaskAutoConfiguration.

Task version updated to 2.1.0

Added missing headers

Updated documentation.

Deprecated EnableTask

Added ability to disable Task autoconfiguration.

Removed @EnableTask from tests

Replaced TaskListenerExecutorFactory with TaskListenerExecutorObjectProviderTests. This delays the need to acquire a bean till it is needed vs at application event time.

Resolves #439
Resolves #440
Resolves #448
Resolves #455
Resolves #461
Resolves #466
Resolves #465

mminella

I also don't see a test confirming that using the annotation (even though it's depricated) does't break anything.

mminella · 2018-10-11T21:02:06Z

...java/org/springframework/cloud/task/batch/handler/TaskJobLauncherCommandLineRunnerTests.java

-		String[] enabledArgs = new String[] { "--spring.cloud.task.batch.failOnJobFailure=true" };
+		String[] enabledArgs = new String[] {
+				"--spring.cloud.task.batch.failOnJobFailure=true",
+				"--spring.batch.job.enabled=false"};// batch job enabled is turned to false so that we can use Task's JobLauncher


Why did the need for this change between Boot 1 and Boot 2?

More importantly, what happened between 2.0 and 2.1 ;-) In short we have 2 beans vying for the JobLauncher, the one provided by boot and the one we provide if the user wants a Task to fail the execution when a batch job fails. Since we don't have bean overriding by default, we have to disable the default boot provided JobLauncher

That doesn't answer what changed...we had that feature before, right? This test implies that going forward, users of this functionality will need to set this property (which is a different behavior from the way we did this before).

We were using bean overriding before. If a user set the failOnJobFailure to true we would override the JobLauncher provided by boot. As of 2.1 we it will not allow that unless we set the flag to re-enable bean overriding or we disable boots default joblauncher.

So we're saying that this breaks the existing functionality?

yep. Unless they add one of the two flags.

Hmmm....We should talk about this.

...loud-task-batch/src/test/java/org/springframework/cloud/task/batch/listener/PrefixTests.java

...g-cloud-task-core/src/main/java/org/springframework/cloud/task/configuration/EnableTask.java

...d-task-core/src/main/java/org/springframework/cloud/task/listener/TaskLifecycleListener.java

spring-cloud-task-core/src/test/java/org/springframework/cloud/task/TaskCoreTests.java

...ask-core/src/test/java/org/springframework/cloud/task/configuration/TaskPropertiesTests.java

spring-cloud-task-docs/src/main/asciidoc/features.adoc

cppwfs · 2018-10-16T17:56:56Z

Added tests that utilize the deprecated @EnableTask.

cppwfs · 2018-10-25T17:38:45Z

Rebased

mminella

Not all of these comments are a direct result of the desired update. Specifically the comments around the BatchStatus vs ExitStatus. That's a miss from the original functionality but we should fix it. The question is if it's part of this PR or another one.

mminella · 2018-10-25T17:56:10Z

...ch/src/main/java/org/springframework/cloud/task/batch/configuration/TaskBatchProperties.java

@@ -43,6 +43,21 @@
 	 */
 	private int commandLineRunnerOrder = 0;

+	/**
+	 * Maximum wait time that Spring Cloud task will wait for tasks to complete


capitalize T on Task.

mminella · 2018-10-25T17:58:14Z

...ch/src/main/java/org/springframework/cloud/task/batch/configuration/TaskBatchProperties.java

+	 * when spring.cloud.task.batch.failOnJobFailure is set to true.  Defaults
+	 * to 5000.
+	 */
+	private long failOnJobFailurePollIntervalInMillis = 5000l;


I'm not a fan of naming a variable with the units. I'd rather document it via javadoc. Do we have other examples within the Spring ecosystem where we take this approach to variable naming?

Yes. https://docs.spring.io/spring/docs/current/javadoc-api/org/springframework/util/StopWatch.html
But in the end, our project may have a different standard.

mminella · 2018-10-25T18:01:40Z

...ingframework/cloud/task/batch/configuration/TaskJobLauncherCommandLineRunnerFactoryBean.java

+	 * when spring.cloud.task.batch.failOnJobFailure is set to true.  Defaults
+	 * to 5000.
+	 */
+	private long failOnJobFailurePollIntervalInMillis = 5000l;


Same points from the properties apply here since these seem to have been copied and pasted. Beyond that, why are we duplicating those fields here and not just referencing the properties values directly? We repeat the values for this in the properties object, the factory bean, and the object itself. Seems pretty redundant to me.

Replace with TaskBatchProperties.

mminella · 2018-10-25T18:47:30Z

...ingframework/cloud/task/batch/configuration/TaskJobLauncherCommandLineRunnerFactoryBean.java

+
+	public void setFailOnJobFailurePollIntervalInMillis(long failOnJobFailurePollIntervalInMillis) {
+		this.failOnJobFailurePollIntervalInMillis = failOnJobFailurePollIntervalInMillis;
+	}


Why getters for properties on a factory bean?

mminella · 2018-10-25T18:55:03Z

...main/java/org/springframework/cloud/task/batch/handler/TaskJobLauncherCommandLineRunner.java

- * Can also be used to launch a specific job by providing a jobName. The
+ * jobs in the surrounding context by default and throw an exception upon the first job
+ * that returns an {@link ExitStatus} of FAILED if a {@link TaskExecutor} is not
+ * specified. If a {@link TaskExecutor} is specified then all Jobs are launched and an


If a TaskExecutor' is specified where (there isn't one in this class)? You really mean if the provided JobLauncherexecutes the jobs in an asynchronous manor, then you get the behavior of all jobs being launched and us throwing the exception if any of them fail vs if theJobLauncher` executes the job in an synchronous manor, then you get the behavior of we check each job as it's executed.

The TaskExecutor is applied to the JobLauncher and this class really never touches it.

To the second question: Yes.

mminella · 2018-10-25T19:40:46Z

...ud-task-batch/src/test/java/org/springframework/cloud/task/batch/listener/TaskBatchTest.java

+import org.springframework.boot.autoconfigure.ImportAutoConfiguration;
+
+/**
+ * Contains the common configurations to run a unit test for the task batch features of


What configurations, specifically, are brought in?

Look in src/test/resources/META-INF/spring.factories

mminella · 2018-10-25T19:43:07Z

...g-cloud-task-core/src/main/java/org/springframework/cloud/task/configuration/EnableTask.java

@@ -54,11 +53,15 @@
 * </ul>
 *
 * @author Glenn Renfro
+ *
+ * @deprecated The EnableTask annotation is no longer be required to initialize
+ * Spring Cloud Task.  This will be handled by Spring Boot AutoConfiguration.


This is handled by the auto configuration provided in Spring Cloud Task via our starter...Spring Boot by itself doesn't handle this.

...src/main/java/org/springframework/cloud/task/listener/TaskListenerExecutorObjectFactory.java

spring-cloud-task-docs/src/main/asciidoc/features.adoc

mminella · 2018-10-25T20:28:48Z

spring-cloud-task-integration-tests/pom.xml

@@ -126,7 +126,7 @@
 		<profile>
 			<id>skipIntegrationTests</id>
 			<activation>
-				<activeByDefault>true</activeByDefault>
+				<activeByDefault>false</activeByDefault>


Why was this switched?

@EnableTask

Migrating to use ApplicationContextRunner or ImportAutoConfiguration with SpringApp.run, instead of SpringApplicationBuilder, because builder does not handle AutoConfiguration properly SimpleTaskAutoConfiguration now has an annotation AutoConfigureBefore the BatchTaskAutoConfig so that it is processed prior. THis is so that that BatchTaskAutoConfig can create the appropriate beans SimpleTaskAutoConfiguration has new annotations so that it is AutoConfigured after BindingServiceConfiguration and after SimpleTaskAutoConfiguration. This is so that it does not attempt to start emitting messages before stream is ready and it can create the appropriate beans after SimpleTaskAutoConfiguration has run. Renamed SimpleTaskConfiguration to SimpleTaskAutoConfiguration. Task version updated to 2.1.0 Added missing headers Updated documentation. Deprecated EnableTask Added ability to disable Task autoconfiguration. Removed @EnableTask from tests Resolves spring-cloud#439 Resolves spring-cloud#440

…tialisation Replaced TaskListenerExecutorFactory with TaskListenerExecutorObjectProviderTests. This delays the need to acquire a bean till it is needed vs at application event time. resolves spring-cloud#448

@EnableTask

Updated to add regression tests for @EnableTask to verify no negative impact when in use.

…LineRunner Used to be a copy of. No just inherits. Added feature check for completed/failed jobs before throwing exception Currently in Task if the user sets failOnJobFailure and they are using a TaskExecutor it will not throw an exception when task fails. This because at evaluation time the job has not started. With the latest change Task waits for all jobs to complete before deciding whether an exception needs to be thrown or not.

* term millis removed from class attributes and moved to docs * timeout attributes removed from TaskJobLauncherCommandLinerRunnerFactory Bean and TaskJobLauncherCommandLineRunner * TaskBatchTest moved to configuration package * Documentation updated * pom.xml reset * rebased conflicts resolved * TaskJobLauncherCommandLineRunner uses BatchStatus instead of exitCode to determine if a Job has failed. resolves spring-cloud#466

…oot 2.1.x The changes to the pom files will need to be removed when Spring Cloud is updated to the latest release. resolves spring-cloud#466

mminella

JobRepository won't work like this. The rest I can handle on merge.

mminella · 2018-10-31T14:23:33Z

...ava/org/springframework/cloud/task/batch/configuration/TaskJobLauncherAutoConfiguration.java

+		JobRepositoryFactoryBean factoryBean = new JobRepositoryFactoryBean();
+		factoryBean.setDataSource(dataSource);
+		return factoryBean.getObject();
+	}


The fact that we need to do this means we're doing something wrong. There should only be one JobRepository for the context. Especially were are assuming default settings which may not be the case. There are more ways to customize a JobRepository implementation than just passing a DataSource that would impact if it even works.

mminella · 2018-10-31T15:44:33Z

...main/java/org/springframework/cloud/task/batch/handler/TaskJobLauncherCommandLineRunner.java

-					Job job = this.jobRegistry.getJob(jobName);
-					if (this.jobs.contains(job)) {
-						continue;
+	private void validateJobExecutions() {


This isn't really validating anything. May rename this method to something more prescriptive.

mminella · 2018-10-31T15:46:11Z

...main/java/org/springframework/cloud/task/batch/handler/TaskJobLauncherCommandLineRunner.java

+		String message = "The following Jobs have failed: \n";
+		for (JobExecution failedJobExecution : failedJobExecutions) {
+			message += String.format("Job %s failed during " +
+					"execution for jobId %s with jobExecutionId of %s \n",


jobId is really JobInstance ID

mminella · 2018-10-31T15:47:10Z

...main/java/org/springframework/cloud/task/batch/handler/TaskJobLauncherCommandLineRunner.java

+		Map<String, JobParameter> merged = new HashMap<>();
+		merged.putAll(parameters.getParameters());
+		merged.putAll(additionals.getParameters());
+		return new JobParameters(merged);


White space is your friend (between methods at a minimum).

mminella · 2018-10-31T15:54:51Z

.../org/springframework/cloud/task/batch/handler/TaskJobLauncherCommandLineRunnerCoreTests.java

@@ -79,12 +90,14 @@

 	@Before
 	public void init() {
+		batchConfigurer.clear();


We're using @DirtiesContext on some tests...beyond this, what else would we need to clear out? Why not just use @DirtiesContext?

mminella · 2018-10-31T15:57:55Z

.../org/springframework/cloud/task/batch/handler/TaskJobLauncherCommandLineRunnerCoreTests.java

+		};
+		Throwable exception = assertThrows(JobRestartException.class, executable);
+		AssertionsForClassTypes.assertThat(exception.getMessage())
+				.isEqualTo("JobInstance already exists and is not restartable");


Why do we run the job twice above, then do a third time? Shouldn't we be able to assert what we need to on the second run?

mminella · 2018-10-31T16:07:48Z

...java/org/springframework/cloud/task/batch/handler/TaskJobLauncherCommandLineRunnerTests.java

+			TaskBatchAutoConfiguration.class,
+			TaskJobLauncherAutoConfiguration.class,
+			SingleTaskConfiguration.class,
+			SimpleTaskAutoConfiguration.class })


Doesn't the @TaskBatchTest annotation bring in a subset of these?

mminella · 2018-10-31T16:10:40Z

...g-cloud-task-core/src/main/java/org/springframework/cloud/task/configuration/EnableTask.java

 @Target(ElementType.TYPE)
 @Retention(RetentionPolicy.RUNTIME)
 @Documented
 @Inherited
-@Import({ SimpleTaskConfiguration.class, SingleTaskConfiguration.class })
+@Import({ })


Why do we have an empty @Import?

mminella · 2018-10-31T16:12:25Z

...d-task-core/src/main/java/org/springframework/cloud/task/listener/TaskLifecycleListener.java

@@ -374,4 +390,5 @@ public int getPhase() {
 	public void destroy() throws Exception {
 		this.doTaskEnd();


Why aren't we removing this line?

mminella · 2018-11-01T15:50:13Z

Rebased, polished, and merged. Thanks for all the work on this one!

cppwfs added the in pr label Aug 15, 2018

cppwfs assigned mminella Aug 15, 2018

cppwfs force-pushed the TASK-439A branch from 706759d to a73bb0b Compare October 2, 2018 17:53

mminella reviewed Oct 12, 2018

View reviewed changes

cppwfs force-pushed the TASK-439A branch from ce67dd5 to 8f9c8b6 Compare October 16, 2018 17:52

cppwfs force-pushed the TASK-439A branch from c00ac51 to 5a01cd3 Compare October 25, 2018 17:38

mminella requested changes Oct 25, 2018

View reviewed changes

cppwfs added 5 commits October 26, 2018 09:29

Updated versions

91cfa0f

TaskLifecycleListener triggers early, and possibly cyclical, bean ini…

1fe94da

…tialisation Replaced TaskListenerExecutorFactory with TaskListenerExecutorObjectProviderTests. This delays the need to acquire a bean till it is needed vs at application event time. resolves spring-cloud#448

Updated based on code review

23d0870

Updated to add regression tests for @EnableTask to verify no negative impact when in use.

cppwfs force-pushed the TASK-439A branch from 5a01cd3 to 53772d8 Compare October 26, 2018 17:06

cppwfs force-pushed the TASK-439A branch from 53772d8 to 6c9bce9 Compare October 26, 2018 21:28

cppwfs added 3 commits October 29, 2018 18:01

Updated TaskJobLauncherCommandLineRunner to match what is in Spring B…

2cab222

…oot 2.1.x The changes to the pom files will need to be removed when Spring Cloud is updated to the latest release. resolves spring-cloud#466

Updated Poms to use the latest Spring Cloud Build

2c5680c

Moved doTaskEnd to stop method instead of destroy method.

be0d83f

mminella reviewed Oct 31, 2018

View reviewed changes

mminella closed this Nov 1, 2018

mminella removed the in pr label Nov 1, 2018

		@@ -374,4 +390,5 @@ public int getPhase() {
		public void destroy() throws Exception {
		this.doTaskEnd();

Update Task to BOOT 2.1.M1 #441

Update Task to BOOT 2.1.M1 #441

Uh oh!

Conversation

cppwfs commented Aug 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mminella left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cppwfs commented Oct 16, 2018

Uh oh!

cppwfs commented Oct 25, 2018

Uh oh!

mminella left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cppwfs Oct 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

cppwfs commented Aug 15, 2018 •

edited

Loading

cppwfs Oct 25, 2018 •

edited

Loading