support continue as new pattern by kaibocai · Pull Request #22 · microsoft/durabletask-java

kaibocai · 2022-04-01T21:58:15Z

This pr includes:

add logics for supporting continue as new pattern
add on end2end test case

refer to #6

kaibocai · 2022-04-01T21:58:52Z

sdk/src/main/java/com/microsoft/durabletask/TaskOrchestrationExecutor.java

            this.historyEventPlayer = new OrchestrationHistoryIterator(pastEvents, newEvents);
        }

+//        private void initialize(){


This method is never invoked, but still pass the test. I am wondering where I made the mistake. @cgillum @davidmrdavid please help.

What were you expected this code would be needed for? I'm not sure if this is actually needed.

The Python SDK has something like this here -https://github.com/Azure/azure-functions-durable-python/blob/2f679e9e6507e7c41195fc752c26e215c532e014/azure/durable_functions/models/TaskOrchestrationExecutor.py#L109

In essence, in Python the initialize method helps "clear" the orchestrator's state when a continue_as_new event is processed by the TaskOrchestrationExecutor.

However, in a later comment you clarify that the Continue_as_new is no longer an event type, which is news to me :) . So if this is already being handled in "the backend", then this is certainly not needed

kaibocai · 2022-04-01T22:00:48Z

sdk/src/main/java/com/microsoft/durabletask/TaskOrchestrationExecutor.java

 //                    break;
-//                case CONTINUEASNEW:
-//                    break;
+                case CONTINUEASNEW:


when debugging the test, never see a event with this type coming. but the test still pass. @cgillum @davidmrdavid please help.

This is expected. Only older versions of the Durable Task Framework actually use this event. Newer versions implement this handling directly in the host. We can remove it from the switch statement.

I see, this is new to me. So is it fair to conclude that, moving forward, OOProc PLs don't need to handle ContinueAsNew history events at all?

Yes, I think it's fair to ignore this moving forward.

cgillum

Glad to see you got this working so quickly! Some feedback:

cgillum · 2022-04-01T22:07:32Z

sdk/src/main/java/com/microsoft/durabletask/TaskOrchestrationContext.java

+
+    void continueAsNew(Object input, boolean preserveUnprocessedEvents );
+
+    void sendEvent(OrchestratorService.OrchestrationInstance instance, String eventName, Object eventData);


Instead of exposing the gRPC types in the public API (OrchestratorService.*), let's just have the user pass in the orchestration instance ID as a String. Something like this:

void sendEvent(String instanceId, String eventName, Object eventData);

The internal implementation of sendEvent can then construct the OrchestrationInstance object.

Also, consider adding a default overload that passes null for eventData.

cgillum · 2022-04-01T22:10:52Z

sdk/src/main/java/com/microsoft/durabletask/TaskOrchestrationExecutor.java

 //                    break;
-//                case CONTINUEASNEW:
-//                    break;
+                case CONTINUEASNEW:


This is expected. Only older versions of the Durable Task Framework actually use this event. Newer versions implement this handling directly in the host. We can remove it from the switch statement.

cgillum · 2022-04-01T22:13:09Z

sdk/src/main/java/com/microsoft/durabletask/TaskOrchestrationExecutor.java

+                // Send all the buffered external events to ourself.
+                OrchestrationInstance.Builder OrchestrationInstanceBuilder = OrchestrationInstance.newBuilder().setInstanceId(this.instanceId);
+                for (EventRaisedEvent unprocessedEvent : unprocessedEvents) {
+                    this.sendEvent(OrchestrationInstanceBuilder.build(), unprocessedEvent.getName(), unprocessedEvent.getInput());


Per my other feedback let's replace OrchestrationInstanceBuilder.build() with this.instanceId.

cgillum · 2022-04-01T22:15:39Z

sdk/src/main/java/com/microsoft/durabletask/TaskOrchestrationExecutor.java

            this.historyEventPlayer = new OrchestrationHistoryIterator(pastEvents, newEvents);
        }

+//        private void initialize(){


What were you expected this code would be needed for? I'm not sure if this is actually needed.

cgillum · 2022-04-01T22:16:13Z

sdk/src/main/java/com/microsoft/durabletask/TaskOrchestrationExecutor.java

        }

+        @Override
+        public void continueAsNew(Object input, boolean preserveUnprocessedEvents ){


nit: fix whitespace here

Suggested change

public void continueAsNew(Object input, boolean preserveUnprocessedEvents ){

public void continueAsNew(Object input, boolean preserveUnprocessedEvents) {

cgillum · 2022-04-01T22:16:34Z

sdk/src/main/java/com/microsoft/durabletask/TaskOrchestrationExecutor.java

        private boolean isComplete;
+        private boolean hasContinueAsNew;
+
+


nit: let's remove this extra whitespace.

cgillum · 2022-04-01T22:19:41Z

sdk/src/main/java/com/microsoft/durabletask/Helpers.java

+    static void throwIfArgumentNullOrWhiteSpace(String argValue, String argName) {
+        throwIfArgumentNull(argValue, argName);
+        if (argValue.trim().length() == 0){
+            throw new IllegalArgumentException("Argument '" + argName + "' was WhiteSpace.");


I would rephrase this slightly:

Suggested change

throw new IllegalArgumentException("Argument '" + argName + "' was WhiteSpace.");

throw new IllegalArgumentException("The argument '" + argName + "' was empty or contained only whitespace.");

cgillum · 2022-04-01T22:26:00Z

sdk/src/main/java/com/microsoft/durabletask/TaskOrchestrationExecutor.java

+                    .build());
+            this.hasContinueAsNew = true;
+
+            if (preserveUnprocessedEvents){


Let's add another integration test that covers this case (unprocessed events). Basically, you should write an orchestration that listens for a single external event and then immediately does ctx.continueAsNew(n + 1). The client should then start an orchestration and, without waiting, immediately raise N external events to the orchestration. For example, 10 events. The client should then wait for the orchestration to complete and verify that the input (and/or output) on the OrchestrationMetadata object is 10, meaning that all 10 external events were processed.

davidmrdavid

I agree with Chris' suggestions. Also left some thoughts and questions, mostly for Chris as well :)

davidmrdavid · 2022-04-01T22:32:05Z

sdk/src/main/java/com/microsoft/durabletask/TaskOrchestrationExecutor.java

+            //TODO: add logic for newVersion
+            CompleteOrchestrationAction.Builder completeOrchestrationActionBuilder = CompleteOrchestrationAction.newBuilder()
+                    .setOrchestrationStatus(OrchestrationStatus.ORCHESTRATION_STATUS_CONTINUED_AS_NEW)
+                    .setNewVersion(StringValue.of(""));


I keep on seeing this version stuff in the actions, which the old out-of-process model did not need. Is this a new requirement, @cgillum?

The Durable Task Framework has a concept of versions of activities and orchestrations. However, we never exposed it to Durable Functions because I couldn't reason about how to leverage it effectively. I went ahead and carried it forward for the OOProc protocols since I want those to work with DTFx but even in the new SDKs we don't actually use them anywhere. At some point we'll want to spend time thinking about how to implement versioning, but we're not there quite yet.

davidmrdavid · 2022-04-01T22:33:15Z

sdk/src/main/java/com/microsoft/durabletask/TaskOrchestrationExecutor.java

 //                    break;
-//                case CONTINUEASNEW:
-//                    break;
+                case CONTINUEASNEW:


I see, this is new to me. So is it fair to conclude that, moving forward, OOProc PLs don't need to handle ContinueAsNew history events at all?

cgillum · 2022-04-05T01:33:50Z

@kaibocai please sync the latest changes from this PR so that you have my updates locally on your machine. It would be sad if you accidentally force-push and delete my changes. :)

Support for "carryover" events (#25)

cgillum

LGTM!

kaibocai commented Apr 1, 2022

View reviewed changes

kaibocai requested review from cgillum and davidmrdavid April 1, 2022 22:01

cgillum reviewed Apr 1, 2022

View reviewed changes

davidmrdavid reviewed Apr 1, 2022

View reviewed changes

kaibocai force-pushed the kaibocai/continue-as-new branch from 8bda6dd to 9216d6e Compare April 4, 2022 21:22

cgillum mentioned this pull request Apr 5, 2022

Support for "carryover" events #25

Merged

kaibocai marked this pull request as ready for review April 5, 2022 01:33

support continue as new pattern

1c2da39

Support for "carryover" events (#25)

kaibocai force-pushed the kaibocai/continue-as-new branch from 23bce59 to 1c2da39 Compare April 5, 2022 02:02

remove repeated code from rebase

9a115ef

kaibocai linked an issue Apr 5, 2022 that may be closed by this pull request

Continue-as-new support #6

Closed

cgillum approved these changes Apr 5, 2022

View reviewed changes

kaibocai merged commit c1f55c2 into main Apr 5, 2022

kaibocai deleted the kaibocai/continue-as-new branch April 5, 2022 17:01

kaibocai requested a review from amamounelsayed April 5, 2022 21:59


		void continueAsNew(Object input, boolean preserveUnprocessedEvents );

		void sendEvent(OrchestratorService.OrchestrationInstance instance, String eventName, Object eventData);

	public void continueAsNew(Object input, boolean preserveUnprocessedEvents ){
	public void continueAsNew(Object input, boolean preserveUnprocessedEvents) {

		private boolean isComplete;
		private boolean hasContinueAsNew;

	throw new IllegalArgumentException("Argument '" + argName + "' was WhiteSpace.");
	throw new IllegalArgumentException("The argument '" + argName + "' was empty or contained only whitespace.");

Conversation

kaibocai commented Apr 1, 2022

Uh oh!

kaibocai Apr 1, 2022 • 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!

Choose a reason for hiding this comment

Uh oh!

cgillum 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!

davidmrdavid 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!

cgillum commented Apr 5, 2022

Uh oh!

cgillum left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

kaibocai Apr 1, 2022 •

edited

Loading