Skip to content

Support reusing orchestration id - #46

Merged
kaibocai (kaibocai) merged 17 commits into
mainfrom
kaibocai/reuse-id-split
Dec 11, 2023
Merged

Support reusing orchestration id#46
kaibocai (kaibocai) merged 17 commits into
mainfrom
kaibocai/reuse-id-split

Conversation

@kaibocai

@kaibocaikaibocai (kaibocai) commented Nov 29, 2023

Copy link
Copy Markdown
Member

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

@kaibocai

kaibocai (kaibocai) commented Nov 29, 2023

Copy link
Copy Markdown
MemberAuthor

Draft the PR, need more tests

@kaibocaikaibocai (kaibocai) mentioned this pull request Nov 29, 2023
Closed
kaibocai
kaibocai (kaibocai) commented Nov 29, 2023
View reviewed changes
Comment threadtests/grpc/grpc_test.go Outdated
@kaibocai
kaibocai (kaibocai) marked this pull request as ready for review November 29, 2023 15:57
cgillum
Chris Gillum (cgillum) reviewed Nov 29, 2023
View reviewed changes

@cgillumChris Gillum (cgillum) left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Happy to see the progress on this. See my comments below. I think we may want to simplify/tighten our implementation of this feature.

Comment threadapi/orchestration.go Outdated
Comment threadapi/orchestration.go Outdated
Comment threadbackend/backend.go Outdated
Comment threadbackend/executor.go Outdated
kaibocai
kaibocai (kaibocai) commented Dec 1, 2023
View reviewed changes
Comment threadbackend/backend.go Outdated
kaibocai
kaibocai (kaibocai) commented Dec 1, 2023
View reviewed changes
Comment threadbackend/sqlite/sqlite.go Outdated
@kaibocai

kaibocai (kaibocai) commented Dec 1, 2023

Copy link
Copy Markdown
MemberAuthor

Chris Gillum (@cgillum) , re-implemented the logic so it doesn't query existing instances at the beginning and most logic goes to backend implementation to utilize the isolation benefit provided by db transaction. The core logic is a little different from the pseudo-code we discussed at #42 (comment), but the effect is the same and more readable.

I assume it still needs a lot of improvement and updates, but would like to get your opinion on the high-level implementation direction. Thanks.

@kaibocai

kaibocai (kaibocai) commented Dec 1, 2023

Copy link
Copy Markdown
MemberAuthor

tagging Alessandro (Ale) Segala (@ItalyPaleAle) for helping review as well. Thank you Ale!

ItalyPaleAle
Alessandro (Ale) Segala (ItalyPaleAle) reviewed Dec 1, 2023
View reviewed changes
Comment thread.github/workflows/pr-validation.yml
Comment thread.github/workflows/pr-validation.yml Outdated
Comment threadapi/orchestration.go Outdated
Comment threadapi/orchestration.go Outdated
Comment threadapi/orchestration.go Outdated
Comment threadbackend/sqlite/sqlite.go Outdated
Comment threadbackend/sqlite/sqlite.go Outdated
Comment threadbackend/sqlite/sqlite.go Outdated
Comment threadbackend/backend.go Outdated
Comment threadtests/grpc/grpc_test.go Outdated
cgillum
Chris Gillum (cgillum) reviewed Dec 1, 2023
View reviewed changes
Comment threadtests/grpc/grpc_test.go Outdated
ItalyPaleAle
Alessandro (Ale) Segala (ItalyPaleAle) reviewed Dec 1, 2023
View reviewed changes
Comment threadbackend/sqlite/sqlite.go Outdated
cgillum
Chris Gillum (cgillum) reviewed Dec 1, 2023
View reviewed changes

@cgillumChris Gillum (cgillum) left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some initial feedback based on reviewing the test code. I'll focus a bit more on the main code once I feel that we have the right tests in place.

Comment threadtests/grpc/grpc_test.go Outdated
Comment threadtests/grpc/grpc_test.go Outdated
Comment threadtests/grpc/grpc_test.go Outdated
Comment threadtests/grpc/grpc_test.go Outdated
Comment threadtests/grpc/grpc_test.go Outdated
Comment threadtests/grpc/grpc_test.go Outdated
cgillum
Chris Gillum (cgillum) reviewed Dec 5, 2023
View reviewed changes

@cgillumChris Gillum (cgillum) left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some feedback on the core design.

Comment threadtests/grpc/grpc_test.go Outdated
Comment threadapi/orchestration.go Outdated
Comment threadapi/orchestration.go Outdated
Comment threadbackend/sqlite/sqlite.go Outdated
Comment threadbackend/sqlite/sqlite.go
Comment threadbackend/sqlite/sqlite.go Outdated
Comment threadbackend/backend.go Outdated
@kaibocai
refactory
a74c223
more refactory
refactory
cgillum
Chris Gillum (cgillum) reviewed Dec 8, 2023
View reviewed changes

@cgillumChris Gillum (cgillum) left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few more comments.

Comment threadapi/orchestration.go Outdated
ErrNotCompleted = errors.New("orchestration has not yet completed")
ErrNoFailures = errors.New("orchestration did not report failure details")
ErrDuplicateInstance = errors.New("orchestration instance already exists")
ErrSkipInstance = errors.New("skip creating orchestration instance")

@cgillumChris Gillum (cgillum)Dec 8, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you using VS Code with the Go extension for editing? Normally it would auto-format the code when you save to line up all the = signs.

Comment threadapi/orchestration.go Outdated
Comment threadbackend/sqlite/sqlite.go Outdated
Comment threadbackend/sqlite/sqlite.go Outdated
Comment threadbackend/backend.go
Comment threadbackend/executor.go
Comment threadtests/backend_test.go Outdated
Comment threadtests/grpc/grpc_test.go Outdated
Comment threadinternal/protos/orchestrator_service.pb.go
Comment threadinternal/protos/orchestrator_service.pb.go Outdated
@kaibocaikaibocai (kaibocai) changed the title Support reuse orchestration idSupport reusing orchestration idDec 11, 2023
cgillum
Chris Gillum (cgillum) approved these changes Dec 11, 2023
View reviewed changes
Comment threadapi/orchestration.go Outdated
ErrNotCompleted = errors.New("orchestration has not yet completed")
ErrNoFailures = errors.New("orchestration did not report failure details")
ErrDuplicateInstance = errors.New("orchestration instance already exists")
ErrIgnoreInstance = errors.New("ignore creating orchestration instance")

@cgillumChris Gillum (cgillum)Dec 8, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like there are still some issues with the whitespace here :(

Comment threadCHANGELOG.md Outdated
The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).

## [v0.3.2] - 2023-12-11

@cgillumChris Gillum (cgillum)Dec 11, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
## [v0.3.2] - 2023-12-11
## [Unreleased]

This was referenced Dec 11, 2023
Closed
Closed
Open
Closed
Merged
Merged
This was referenced Jan 5, 2024
Open
Closed
Open
Closed
Closed
Closed
Closed
@kaibocai
kaibocai (kaibocai) deleted the kaibocai/reuse-id-split branch January 16, 2024 20:01
Fabian Martinez (famarting) pushed a commit to famarting/durabletask-go that referenced this pull request Oct 3, 2025
@JoshVanL
Merge pull requestmicrosoft#46from famarting/ignored-errors
99d7903
explicitly ignore unhandled errors
CopilotAI mentioned this pull request Jan 5, 2026
Draft
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cgillumChris Gillum (cgillum)cgillum approved these changes
+1 more reviewer
@ItalyPaleAleAlessandro (Ale) Segala (ItalyPaleAle)ItalyPaleAle approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@kaibocai@ItalyPaleAle@cgillum