Skip to content

Conversation

yycptt
Copy link
Member

@yycptt yycptt commented May 1, 2025

What changed?

  • Implement chasm engine Update/Read Component method

Why?

  • CHASM work stream

How did you test it?

  • built
  • run locally and tested manually
  • covered by existing tests
  • added new unit test(s)
  • added new functional test(s)

@yycptt yycptt requested review from alexshtin and lina-temporal May 1, 2025 21:23
Copy link
Contributor

@lina-temporal lina-temporal left a 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)

Copy link
Contributor

@alexshtin alexshtin left a 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)
Copy link
Contributor

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?

Copy link
Member Author

@yycptt yycptt May 8, 2025

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.

@yycptt yycptt force-pushed the chasm-engine-update-read branch from 0345dba to 9ced21d Compare May 8, 2025 01:15
@yycptt yycptt marked this pull request as ready for review May 8, 2025 01:16
@yycptt yycptt requested a review from a team as a code owner May 8, 2025 01:16
Copy link
Contributor

@lina-temporal lina-temporal left a comment

Choose a reason for hiding this comment

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

LGTM!

@yycptt yycptt force-pushed the chasm-engine-update-read branch from 9ced21d to 531de45 Compare May 8, 2025 22:42
@yycptt yycptt merged commit 8c5e61e into temporalio:main May 9, 2025
52 checks passed
@yycptt yycptt deleted the chasm-engine-update-read branch May 9, 2025 02:21
josesa added a commit to josesa/temporal that referenced this pull request May 12, 2025
* 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)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants