-
Notifications
You must be signed in to change notification settings - Fork 1.1k
CHASM: Engine Update/ReadComponent implementation #7696
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just some questions and minor suggestions, otherwise LGTM (I'm assuming it's draft before the tests are ready)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe some unit tests won't hurt here, but because I am going to add e2e tests soon, I guess, you might skip them. registry.go
had 100% coverage before though 🙂.
executionLease.GetReleaseFn()(nil) | ||
}() | ||
|
||
chasmTree, ok := executionLease.GetMutableState().ChasmTree().(*chasm.Node) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand this cast. Why not to change NewContext
to accept ChasmTree
interface (instead of *Node
) and add missing methods to ChasmTree
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
change NewContext to accept ChasmTree interface
That's a cycle dependency. ChasmTree
is in historyi
package which depends on chasm
.
I can move the chasmTree interface, but that interface and the noop tree impl will eventually be removed, so I guess I will just go with the simplest type assertion approach now.
0345dba
to
9ced21d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
9ced21d
to
531de45
Compare
* main: (22 commits) Add host health metrics gauge (temporalio#7728) add rule expiration check (temporalio#7749) Add activity options to the pending activity info (temporalio#7727) Enable DLQ V2 for replication (temporalio#7746) chore: be smarter about when to use Stringer vs String (temporalio#7743) versioning entity workflows: enabling auto-restart pt1 (temporalio#7715) Refactor code generators (temporalio#7734) allow passive to generate replication tasks (temporalio#7713) Validate links in completion callbacks (temporalio#7726) CHASM: Engine Update/ReadComponent implementation (temporalio#7696) Enable transition history in dev env and tests (temporalio#7737) chore: Add Stringer tags (temporalio#7738) Add internal pod health check to DeepHealthCheck (temporalio#7709) Rename internal CHASM task processing interface (temporalio#7730) [Frontend] Log slow gRPC requests (temporalio#7718) Remove cap for dynamic config callback pool (temporalio#7723) Refactor updateworkflowoptions package (temporalio#7725) Remove a bunch of redundant utf-8 validation (temporalio#7720) [CHASM] Pure task processing - GetPureTasks, ExecutePureTasks (temporalio#7701) Send ActivityReset flag to the worker in heartbeat response (temporalio#7677) ...
What changed?
Why?
How did you test it?