Support Orchestration ID Reuse Policies #45

kaibocai · 2023-11-21T19:26:49Z

This PR tries to update the logic to support reuse orchestration ID, more details can be found #42, dapr/dapr#7101

Corresponding protobuf updates can be found microsoft/durabletask-protobuf#19
It adds enum InstanceExistOption which contains three options THROW_IF_EXIST, TERMINATE_IF_EXIST, SKIP_IF_EXIST.
THROW_IF_EXIST will be the default behavior which is also the current behavior.

cgillum

Some initial feedback. One high level point is that I'd like it if we can try to organize this in a way that reduces duplicate code when we need to support multiple backends.

cgillum · 2023-11-21T20:21:31Z

backend/backend.go

@@ -16,6 +16,7 @@ var (
 	ErrNotInitialized        = errors.New("backend not initialized")
 	ErrWorkItemLockLost      = errors.New("lock on work-item was lost")
 	ErrBackendAlreadyStarted = errors.New("backend is already started")
+	ErrSkipExistInstance 	 = errors.New("Skip creating existing instance")


nit: golang style guides say that error messages should start with a lowercase letter. I see warnings in VS Code when I try to create error messages that start with capital letters.

cgillum · 2023-11-21T20:24:24Z

backend/sqlite/sqlite.go

@@ -403,6 +403,9 @@ func (be *sqliteBackend) CreateOrchestrationInstance(ctx context.Context, e *bac

 	var instanceID string
 	if err := be.createOrchestrationInstanceInternal(ctx, e, tx, &instanceID); err != nil {
+		if err == backend.ErrSkipExistInstance {


In Go, it's preferred to do errors.Is(err, backend.ErrSkipExistinstance) instead of err == backend.ErrSkipExistInstance. More info here: https://go.dev/blog/go1.13-errors.

Suggested change

if err == backend.ErrSkipExistInstance {

if errors.Is(err, backend.ErrSkipExistinstance) {

cgillum · 2023-11-21T20:25:58Z

backend/sqlite/sqlite.go

@@ -403,6 +403,9 @@ func (be *sqliteBackend) CreateOrchestrationInstance(ctx context.Context, e *bac

 	var instanceID string
 	if err := be.createOrchestrationInstanceInternal(ctx, e, tx, &instanceID); err != nil {
+		if err == backend.ErrSkipExistInstance {
+			return nil


We should log a warning message here. Something like: "An instance with ID '%s' already exists; dropping duplicate create request"

cgillum · 2023-11-21T20:34:12Z

backend/sqlite/sqlite.go

+		instanceExists = true
+	}
+
+	if !instanceExists || startEvent.InstanceExistOption == protos.InstanceExistOption_Throw_IF_EXIST {


As far as I can tell, this won't actually throw if the instance doesn't exist. The query below is INSERT OR IGNORE... which means that the query will just return 0 results if the instance already exists, and we'll return ErrDuplicateEvent.

Yes, with ErrDuplicateEvent send back through grpc, from client side we get an exception as

wondering does this count as throw

Ah, yes, I think I got confused. My suggestion is that we keep the existing logic as-is except for the TERMINATE_IF_EXIST case. Instead of handling THROW_IF_EXIST or SKIP_IF_EXIST here, we can simply update the caller to check for ErrDuplicateEvent. If we get ErrDuplicateEvent and the policy is THROW_IF_EXIST, then we can return the error as we do today. Otherwise, it the policy is SKIP_IF_EXIST, we can just log a warning and return a success. I think it's better to have that logic in the calling code so that we can reduce the amount of special handling the different backends need to implement.

cgillum · 2023-11-21T20:34:34Z

backend/sqlite/sqlite.go

-		) VALUES (?, ?, ?, ?, ?, ?, ?)`,
-		startEvent.Name,
-		startEvent.Version.GetValue(),
+		`SELECT * FROM [Instances] WHERE [InstanceID] = ?`, 


Do we need this new SQL statement or is the existing logic sufficient?

cgillum · 2023-11-21T20:36:24Z

backend/sqlite/sqlite.go

-	*instanceID = startEvent.OrchestrationInstance.InstanceId
+		*instanceID = startEvent.OrchestrationInstance.InstanceId
+	} else if startEvent.InstanceExistOption == protos.InstanceExistOption_SKIP_IF_EXIST {
+		be.logger.Debugf(


Instead of logging here, I suggest we log in the calling code. That way, we have common logging behavior that works for all backend types. I want to minimize the amount of duplication we have to do across all backends (sqlite, actors, postgres, custom, etc.)

One question about the Backend interface, if I want to update it will it be a break change for Dapr workflow, I haven't really look into how Dapr is using durabletask-go, I assume it implements the Backend interface but using actor storage. So is it ok to update this interface?

I don't think my suggestion requires a breaking change, but it's okay for us to make breaking changes still since durabletask-go is not consider "stable". Yes, we'll need to update the Dapr implementation, but that's fine - we've done it many times before. It doesn't impact customers.

cgillum · 2023-11-21T20:37:04Z

backend/sqlite/sqlite.go

+		// same instance id exists and InstanceExistOption_TERMINATE_IF_EXIST set
+		// to terminate old instance
+		// 1. remove all entires in history table
+		// 2. remove all entries in instance table


We just need to remove the instance with the same ID, not "all" instances.

cgillum · 2023-11-21T20:37:53Z

backend/sqlite/sqlite.go

+		// 3. remove all entries in newEvents table
+		// 4. remove all entries in new task table
+		// 5. insert new instance to instance table. 
+		// do all above updates in one atomic operation


We're already operating in the context of a transaction, so it's okay if you do this in multiple discrete steps.

kaibocai · 2023-11-23T00:59:26Z

Just add a more concrete implementation, but also got two questions

Question: from the customer side SKIP_IF_EXIST and TERMINATE_IF_EXIST have the same behavior if success. Do customers need to know the difference between them? SKIP_IF_EXIST uses the old instance, whereas TERMINATE_IF_EXIST terminates the old instance and creates a new instance with the same instance ID.

Seems this client file is only used for test, do we also need to update it's schedule instance logic at

durabletask-go/backend/client.go

Line 40 in ba82393

    
           func (c *backendClient) ScheduleNewOrchestration(ctx context.Context, orchestrator interface{}, opts ...api.NewOrchestrationOptions) (api.InstanceID, error) {

kaibocai · 2023-11-23T01:00:24Z

backend/backend.go

@@ -16,6 +16,7 @@ var (
 	ErrNotInitialized        = errors.New("backend not initialized")
 	ErrWorkItemLockLost      = errors.New("lock on work-item was lost")
 	ErrBackendAlreadyStarted = errors.New("backend is already started")
+	ErrInstanceNotExists	 = errors.New("isntance does not exist")


Suggested change

ErrInstanceNotExists = errors.New("isntance does not exist")

ErrInstanceNotExists = errors.New("instance does not exist")

kaibocai · 2023-11-29T03:06:47Z

Open a new PR #46, closing this one.

kaibocai changed the title ~~Kaibocai/reuse~~ Support Orchestration ID Reuse Policies Nov 21, 2023

kaibocai requested a review from cgillum November 21, 2023 19:27

cgillum reviewed Nov 21, 2023

View reviewed changes

add complete implmentation

f5b9aac

kaibocai force-pushed the kaibocai/reuse-id branch from 0676d38 to f5b9aac Compare November 23, 2023 00:54

kaibocai commented Nov 23, 2023

View reviewed changes

kaibocai added 7 commits November 27, 2023 16:58

update pipeline to generate grpc code

49b86ad

manually install protoc-gen-go

b3ccf91

update correc download url

df49e31

pipeline minor udpates

e758af7

minor updates on pipeline

3f9be92

update pipeline

de38c65

regenerate mocks code

eaf19c1

kaibocai closed this Nov 29, 2023

kaibocai deleted the kaibocai/reuse-id branch January 16, 2024 20:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support Orchestration ID Reuse Policies #45

Support Orchestration ID Reuse Policies #45

kaibocai commented Nov 21, 2023 •

edited

Loading

cgillum left a comment

cgillum Nov 21, 2023

cgillum Nov 21, 2023

cgillum Nov 21, 2023

cgillum Nov 21, 2023

kaibocai Nov 21, 2023

cgillum Nov 22, 2023

cgillum Nov 21, 2023

cgillum Nov 21, 2023

kaibocai Nov 21, 2023

cgillum Nov 22, 2023

cgillum Nov 21, 2023

cgillum Nov 21, 2023

kaibocai commented Nov 23, 2023

kaibocai Nov 23, 2023

kaibocai commented Nov 29, 2023

	if err == backend.ErrSkipExistInstance {
	if errors.Is(err, backend.ErrSkipExistinstance) {

	ErrInstanceNotExists = errors.New("isntance does not exist")
	ErrInstanceNotExists = errors.New("instance does not exist")

Support Orchestration ID Reuse Policies #45

Support Orchestration ID Reuse Policies #45

Conversation

kaibocai commented Nov 21, 2023 • edited Loading

cgillum left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kaibocai commented Nov 23, 2023

Choose a reason for hiding this comment

kaibocai commented Nov 29, 2023

kaibocai commented Nov 21, 2023 •

edited

Loading