Skip to content

Conversation

@lina-temporal
Copy link
Contributor

@lina-temporal lina-temporal commented May 8, 2025

What changed?

  • Added ExecuteSideEffectTask to Node, as with ExecutePureTask.

Why?

  • Facilitates the remaining physical task executors.

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)

@lina-temporal lina-temporal requested review from alexshtin and yycptt May 8, 2025 00:02
@lina-temporal lina-temporal requested a review from a team as a code owner May 8, 2025 00:02
@lina-temporal lina-temporal marked this pull request as draft May 8, 2025 00:13
@lina-temporal lina-temporal force-pushed the chasm_tree_side_effect branch from 13ab36a to 897f4f8 Compare May 8, 2025 19:21
@lina-temporal lina-temporal changed the base branch from main to export_engine June 3, 2025 02:36
@lina-temporal lina-temporal changed the base branch from export_engine to main June 3, 2025 02:37
@lina-temporal lina-temporal marked this pull request as ready for review June 3, 2025 02:40
@lina-temporal
Copy link
Contributor Author

Once #7855 is approved and lands in main, I'll merge main here and that should cut down on the boilerplate noise.

Copy link
Member

@yycptt yycptt left a comment

Choose a reason for hiding this comment

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

Change for transfer & outbound queue will be in a separate PR?

task,
t.getCurrentTime,
t.config.StandbyTaskMissingEventsDiscardDelay(task.GetType()),
t.checkWorkflowStillExistOnSourceBeforeDiscard,
Copy link
Member

Choose a reason for hiding this comment

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

hmm can't really use this or at least need to change it's implementation. It's calls DescribeWorkflowExecution() which is specific for workflow.

Can you add a TODO here and create an issue to track this? No super urgent but need to get it fixed before we run chasm in production

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmm can't really use this or at least need to change it's implementation. It's calls DescribeWorkflowExecution() which is specific for workflow.

Given that CHASM entity keys map cleanly back to workflow keys and are still written to DB as workflow executions, what prevents DescribeWorkflowExecution from working here?

Copy link
Member

Choose a reason for hiding this comment

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

It's a workflow specific API and should return notFound if the requested ID is not a Workflow. Or maybe a different error type say InvalidArgument, still in that case needs to at least update the error handling logic in checkWorkflowStillExistOnSourceBeforeDiscard

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Gotcha, adding a TODO and a task.

@lina-temporal lina-temporal requested a review from yycptt June 9, 2025 20:16
@lina-temporal lina-temporal enabled auto-merge (squash) June 10, 2025 23:09
@lina-temporal lina-temporal merged commit 32b466f into main Jun 10, 2025
53 checks passed
@lina-temporal lina-temporal deleted the chasm_tree_side_effect branch June 10, 2025 23:30
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