Add initial Spring Boot implementation #1305

Spikhalskiy · 2022-07-12T06:02:44Z

What was changed

Add an initial barebone Spring boot module that allows simple wiring of workflow and activities with minimal code.
The main goal of this PR is to provide an MVP framework for further improvements. The current state is not production-ready and postfixed with -alpha
More configuration and deep integration with Spring DI will be coming in subsequent PRs for the Spring Boot story.

One of the goals of this PR is not touching code in core temporal-sdk modules for simplicity of review. All changes that will require changes to core temporal-sdk modules will be coming in separate easier-to-manage PRs.

This PR provides an MVP for #8 and a supporting ground for improvements roughly outlined in #745.

cretz · 2022-07-14T12:40:20Z

...igure-alpha/src/main/java/io/temporal/spring/boot/autoconfigure/ClientAutoConfiguration.java

+                  + "and configured `spring.temporal.testServer.enabled: true`.");
+        }
+        return testWorkflowEnvironment.getWorkflowClient();
+      case ClientProperties.TARGET_LOCAL_SERVICE:


What is the difference between this and me explicitly providing localhost:7233? Can the const just be that and we don't have a case for it? Or is it that important to reuse the stub instance?

There is none, but we have WorkflowServerStub.newLocalStubs, so I think it would be right to mirror it here.
The main argument for this is that it's much friendlier for new users to don't even worry about which port Temporal occupies by default.

cretz · 2022-07-14T12:44:48Z

...igure-alpha/src/main/java/io/temporal/spring/boot/autoconfigure/ClientAutoConfiguration.java

+                stubsOptionsBuilder.validateAndBuildWithDefaults());
+    }
+
+    WorkflowClientOptions.Builder clientOptionsBuilder = WorkflowClientOptions.newBuilder();


I assume configure for other options can come later? E.g. can I "inject" (or whatever spring calls it these days) an interceptor or data converter?

"More configuration and deep integration with Spring DI will be coming in subsequent PRs for the Spring Boot story."

The purpose of this PR is to provide a correct structure that will initialize things in the correct order and with reasonable logic. You can't configure almost anything not absolutely essential here yet.

cretz · 2022-07-14T12:49:28Z

...gure-alpha/src/main/java/io/temporal/spring/boot/autoconfigure/WorkersAutoConfiguration.java

+        for (String taskQueue : annotation.taskQueues()) {
+          log.info(
+              "Registering auto-discovered workflow class {} on a task queue {}", clazz, taskQueue);
+          Worker worker = workerFactory.newWorker(taskQueue);


We should reuse workers based on task queue I believe. If I am reading correct, code as it is now could have a worker configured for task queue A, an annotation-based workflow for task queue A, and another annotation-based workflow for task queue A, and this will create three workers which will cause an issue.

and this will create three workers which will cause an issue.

newWorker reuses workers. With warning in log though. It never was possible to create several workers on the same task queue on the same WorkerFactory.

This code be changed later. I didn't want to touch SDK code in this PR and right now there is no getWorker on WorkerFactory, while there should be. Will be added is a separate PR.

newWorker reuses workers. With warning in log though.

I think workers should be reused here with no warning. I should be able to create two WorkflowImpls in the same package with the same task queue without a warning I think. Of course if you configure two workers w/ the same task queue, it should error I think.

Not possible with the current API. I will be adding a new API for WorkerFactory that will allow doing that.

I am suggesting doing it in this method. Something like (untested, just off the top of my head here in comment) for the first part:

Map<String, Worker> workers = properties.getWorkers().stream() .collect(Collectors.toMap( props -> props.getTaskQueue(), props -> newWorker(workerFactory, props), (tq1, tq2) -> { throw InvalidArgumentException("Multiple configured workers with task queue: " + tq1); }));

And for the annotation part:

if (autoDiscoveredWorkflowImplementationClasses != null) { for (Class<?> clazz : autoDiscoveredWorkflowImplementationClasses) { WorkflowImpl annotation = clazz.getAnnotation(WorkflowImpl.class); for (String taskQueue : annotation.taskQueues()) { log.info( "Registering auto-discovered workflow class {} on a task queue {}", clazz, taskQueue); Worker worker = workers.computeIfAbsent(taskQueue, workerFactory::newWorker); configureWorkflowImplementation(worker, clazz); } } }

I don't see a reason for WorkerFactory shouldn't expose helper methods that make it easier. Our users should have an easier time implementing code like that themselves if they want it. workers map is already inside WorkerFactory. I will prefer to improve WorkerFactory interface for better flexibility.

Yeah, if we error on multiple configured workers of the same task queue, that's fine.

cretz · 2022-07-14T12:56:13Z

...gure-alpha/src/main/java/io/temporal/spring/boot/autoconfigure/WorkersAutoConfiguration.java

+    Set<Worker> workers =
+        properties.getWorkers().stream()
+            .map(workerProperties -> newWorker(workerFactory, workerProperties))
+            .collect(Collectors.toSet());


May want to do a quick pass through here to make sure there aren't two workers configured for the same task queue

It shouldn't be the responsibility of SpringBoot to ensure this. WorkerFactory should never allow the creation of several workers for the same task queue for quite a bit of reasons.

Per #1305 (comment), I just think that it should be an error to configure two workers for the same task queue (and otherwise reuse silently).

If you are talking about verifying integrity of the user configuration - yeah, it makes sense.

cretz · 2022-07-14T12:57:25Z

...gure-alpha/src/main/java/io/temporal/spring/boot/autoconfigure/WorkersAutoConfiguration.java

+            (Class<T>) workflowInterface.getInterfaceClass(),
+            () -> (T) beanFactory.createBean(clazz));
+        hasWorkflowMethod = true;
+        break;


Or just return here and remove hasWorkflowMethod variable and always throw after the loop

This code is nasty, it will go away into the core module into the classes that do reflection lookup. It just made it work right now, this definitely will be cleaned up.

cretz · 2022-07-14T13:00:11Z

...c/main/java/io/temporal/spring/boot/autoconfigure/WorkersAutoDiscoveryAutoConfiguration.java

+        }
+      }
+    }
+    return implementations;


I wonder if you want to find name + task queue clashes here or let it fail at worker validation time.

It will fail on registration on the workers. I will try to not bring logic like this into spring boot as long as possible. What can be shared, should be shared. The only reason why it can leak here is if we want to give some super-specific error message that can reference a spring boot config.

cretz · 2022-07-14T13:03:45Z

...alpha/src/main/java/io/temporal/spring/boot/autoconfigure/properties/TemporalProperties.java

+import org.springframework.boot.context.properties.ConstructorBinding;
+import org.springframework.boot.context.properties.NestedConfigurationProperty;
+
+@ConfigurationProperties(prefix = "spring.temporal")


Can you think of a use case to wrap all of these properties as a collection and bring this one level higher? (or is there a layer in Spring that lets that already happen?)

For example, what if I want to auto configure two workers with separate clients? With this setup, can I have two workflows on one namespace and two other workflows on another namespace?

Alternatively we could think about making clients and workers accept namespaces so I could provide namespaces alongside taskQueues in my @WorkflowImpl (or maybe workerName in the annotation instead, unsure). But meh, we can stick to not supporting same-task-queue-different-namespace.

I thought about it a little bit and it's an interesting question. I didn't find an elegant solution yet and I'm gonna tackle it in a separate PR.

Here is the problem. We have too many moving parts.

We have namespaces. So, ideally, we should allow setting this whole config per-namespace (collection as you said). WorkflowImpl and ActivityImpl will have to have a pair namespace:taskQueue for auto-discovery after that. Things become a bit more complex, but in a manageable manner. This is on me to-do list of next things.

Multiple clients. I think it's not really worth the complexity. I do have a to-do in mind on supporting separate client and worker clients in the next PR. Now we are cheating and just have a worker client, but it's not the correct approach. We should have a separate workerClient and well, clientClient :) . I probably should've done this part in this PR to establish a correct structure, but I expect it to be a messy and frustrating task of its own and it will be in a separate PR. To do it well, I need to come up with a way for worker and client Clients to share the majority of the config.

👍 Is it possible to mark this Spring Boot stuff as experimental and subject to incompatible changes in the near term?

Already have it in the readme and the module names have -alpha postfix. I hope it's loud enough, if you have any other ideas to do it - share.

Works for me.

cretz

Approving assuming we can still make backwards incompatible changes to this Spring Boot API for a while in the next sets of PRs

bergundy

I didn't go into the implementation details but the usage part looks great

Spikhalskiy force-pushed the spring-boot-alpha branch 19 times, most recently from 2a21c7b to f59860c Compare July 14, 2022 02:38

Spikhalskiy marked this pull request as ready for review July 14, 2022 03:00

Spikhalskiy requested review from mfateev, Sushisource, cretz, bergundy and mmcshane as code owners July 14, 2022 03:00

Spikhalskiy force-pushed the spring-boot-alpha branch from f59860c to a03a78e Compare July 14, 2022 03:10

Add initial Spring Boot implementation

79def99

Spikhalskiy force-pushed the spring-boot-alpha branch from a03a78e to 79def99 Compare July 14, 2022 05:49

cretz reviewed Jul 14, 2022

View reviewed changes

cretz approved these changes Jul 14, 2022

View reviewed changes

Spikhalskiy merged commit 9952b0d into temporalio:master Jul 14, 2022

Spikhalskiy deleted the spring-boot-alpha branch July 14, 2022 16:41

bergundy reviewed Jul 14, 2022

View reviewed changes

Spikhalskiy added this to the 1.15.0 milestone Jul 14, 2022

Spikhalskiy mentioned this pull request Aug 16, 2022

Spring Boot Integration #8

Closed

13 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add initial Spring Boot implementation #1305

Add initial Spring Boot implementation #1305

Spikhalskiy commented Jul 12, 2022 •

edited

Loading

cretz Jul 14, 2022 •

edited

Loading

Spikhalskiy Jul 14, 2022

cretz Jul 14, 2022

Spikhalskiy Jul 14, 2022

cretz Jul 14, 2022

Spikhalskiy Jul 14, 2022 •

edited

Loading

cretz Jul 14, 2022

Spikhalskiy Jul 14, 2022

cretz Jul 14, 2022 •

edited

Loading

Spikhalskiy Jul 14, 2022 •

edited

Loading

cretz Jul 14, 2022

cretz Jul 14, 2022

Spikhalskiy Jul 14, 2022

cretz Jul 14, 2022

Spikhalskiy Jul 14, 2022

cretz Jul 14, 2022

Spikhalskiy Jul 14, 2022

cretz Jul 14, 2022

Spikhalskiy Jul 14, 2022

cretz Jul 14, 2022

Spikhalskiy Jul 14, 2022 •

edited

Loading

cretz Jul 14, 2022

Spikhalskiy Jul 14, 2022

cretz Jul 14, 2022

cretz left a comment

bergundy left a comment

Add initial Spring Boot implementation #1305

Add initial Spring Boot implementation #1305

Conversation

Spikhalskiy commented Jul 12, 2022 • edited Loading

What was changed

cretz Jul 14, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Spikhalskiy Jul 14, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cretz Jul 14, 2022 • edited Loading

Choose a reason for hiding this comment

Spikhalskiy Jul 14, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Spikhalskiy Jul 14, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cretz left a comment

Choose a reason for hiding this comment

bergundy left a comment

Choose a reason for hiding this comment

Spikhalskiy commented Jul 12, 2022 •

edited

Loading

cretz Jul 14, 2022 •

edited

Loading

Spikhalskiy Jul 14, 2022 •

edited

Loading

cretz Jul 14, 2022 •

edited

Loading

Spikhalskiy Jul 14, 2022 •

edited

Loading

Spikhalskiy Jul 14, 2022 •

edited

Loading